Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to add drivers to a driver chain #35

Merged
merged 1 commit into from
Oct 15, 2013

Conversation

bakura10
Copy link
Contributor

In my specific case, each driver needs to have a reference to the metadata factory, and to create the metadata factory I need... the driver chain. So I'm stuck. Adding this method add a bit of flexibility to the DriverChain.

schmittjoh added a commit that referenced this pull request Oct 15, 2013
Allow to add drivers to a driver chain
@schmittjoh schmittjoh merged commit fc4406c into schmittjoh:master Oct 15, 2013
@bakura10 bakura10 deleted the patch-1 branch October 15, 2013 10:32
@bakura10
Copy link
Contributor Author

Thanks, tell me when you tag :).

@schmittjoh
Copy link
Owner

Hm yeah, we don't have terribly many changes yet :)

btw, I'm thinking about whether it would make sense to add a MetadataFactoryAware interface so that the factory injects itself into drivers if necessary. This would stream-line the construction of the objects.

However, before we would do that I'd like to learn a bit more why you inject the metadata factory. I presume you want to recursively load metadata?

@bakura10
Copy link
Contributor Author

Exactly. I may have multiple drivers, and when a driver create an object it may needs to get the data from the factory and merge data.

@schmittjoh
Copy link
Owner

Ah, I see. Is the code open-source, so that I could take a look at it?

@bakura10
Copy link
Contributor Author

Hi @schmittjoh ,

After implementing my custom factory, I encounter an annoying problem, and I have no idea about how to solve it. I have a nice circular dependency problem here: https://github.com/zf-fr/zfr-rest/blob/voltaire/src/ZfrRest/Resource/Metadata/Driver/AnnotationDriver.php#L109

Basically, this happens when using a bi-directional associations between two resources. I load the resource metadata for resource A, that loads metadata for resource B, which itself loads metadata for resource A and... problem!

I can't avoid the bi-directional here, because mapping can be overridden by an association. Do you have any idea about how I could solve this issue?

@schmittjoh
Copy link
Owner

What result would you expect? Do you have an example where I can see where this metadata is coming from, i.e. the annotated objects?

@bakura10
Copy link
Contributor Author

Yes, annotated objects.

I have made a simple failing tests:

and the failing test: https://github.com/zf-fr/zfr-rest/blob/voltaire/tests/ZfrRestTest/Resource/Metadata/Driver/AnnotationDriverFunctionalTest.php#L103

Basically, when there is an association, the mapping defines on the association must override mappins define globally on the associated resource. Not sure if it makes sense :o.

@schmittjoh
Copy link
Owner

What I could think of is that you defer the resolution of that metadata. Basically, instead of trying to load the metadata right away in the driver, you would simply add a note to the class metadata that an association still needs to be resolved.

Then, you could wrap the MetadataFactory with your own implementation which would check all loaded metadata whether they contain associations which need to be resolved, and resolve them then. I think this would also be a cleaner implementation as the resolution would not have to be implemented in each driver (if you for example have an annotation driver, a yaml driver, etc.).

Would that work?

@bakura10
Copy link
Contributor Author

The custom metadata factory seems the best solution! I'll try that :).

@bakura10
Copy link
Contributor Author

Haaa this is not currently possible because MetadataFactory is final :/. I'm going to make a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants