Advanced metadata implementation. #20
Conversation
|
Tests pass locally, it looks like Travis is failing this due to a missing rakefile. I can review later. |
|
Yeah, I think you can put the implemention for I'm also not yet sure about the name |
|
@jstout24 the actual reason of the travis failure is because Btw @schmittjoh, why is travis enabled if there is no config ? |
| * | ||
| * @author Jordan Stout <j@jrdn.org> | ||
| */ | ||
| class AdvancedDriverChain extends DriverChain implements AdvancedDriverInterface |
There was a problem hiding this comment.
I don't think you need 2 separate DriverChain. This could be directly in the DriverChain (it is fully BC and does not force to add only advanced drivers in it)
There was a problem hiding this comment.
Gotcha, yes, I went overboard here. I was quickly trying to get an idea out there before I spent too much time on it so I overlooked the DriverChain itself since I was on a file creating streak ;P
|
@schmittjoh I agree on the naming. I feel that all this logic should belong within the classes themselves, but to keep BC, creating extending classes seems to be the only way to go, however, I too don't know what to name it. Will keep it "Advanced" just to move forward. @stof thanks for reviewing! i'll get on all the changes and try to get a more concrete PR. |
|
The more I work on it, the more I feel this logic belongs in the classes themselves... Not interested in introducing 1.2.0? :P |
|
I'll rebase and fix everything later, for now, I'll commit piece by piece to make sense of all the changes. Anyway, @schmittjoh you're in a way different timezone than I am, so if you want to work on top of this or implement it yourself if you feel that I'm way off, feel free to. I'm hoping to get this functionality soon so I can finish my current project before the holidays and this is the last thing holding me back :( I ended up keeping some of the interfaces and just having the classes implement them which in turn implement the "basic" interfaces (because in some cases I want to just use the interface.. for example https://github.com/schmittjoh/serializer/blob/master/src/JMS/Serializer/Metadata/Driver/AnnotationDriver.php). Maybe we can keep it this way and eventually bring the two together in a new release if you feel this belongs in the core of the classes. |
| { | ||
| $classes = array(); | ||
| foreach ($this->drivers as $driver) { | ||
| if ($driver instanceof AdvancedDriverInterface) { |
There was a problem hiding this comment.
Should we throw an exception if a driver is not implementing the AdvancedDriverInterface? After all, you won't get all class names anymore as soon as there is a driver which does not implement it.
There was a problem hiding this comment.
I'm +1 for this, but as @stof mentioned that DriverChain shouldn't force advanced drivers, I did the check instead of an exception.
I see plusses in both situations, but feel that it may cause more problems if we don't add an exception here since the most common use case is to warm-up a cache, or work with all configured metadata (as I am doing).
Let me know what you feel is best and I can include it.
There was a problem hiding this comment.
Alright, let's use an exception then.
|
Apart from my comment above, the code looks good. I'd like to revert the visibility changes, removals of final though unless they are needed. In terms of BC, it also looks good to me since all interfaces that we introduced are automatically implemented by the respective classes, so there should be no errors afaics. |
|
Yep, I'll fix all you've requested and put the finals back as I made the abstract and common classes implement advanced (which shouldn't break BC). All the code in this PR was only tested through the unit tests, but tonight I've been playing with it in the project that I actually needed it's functionality and it works like a charm! 🏄 It's late here and I have work in the AM, but I'll get on these changes tomorrow morning (so your next morning I guess 👅) |
|
Oh and I'll clean up the commits before you merge this. |
|
Looking forward to it :) |
|
Woot! I see green now! :P |
| public function getAllClassNames() | ||
| { | ||
| if (!$this->locator instanceof AdvancedFileLocatorInterface) { | ||
| return array(); |
There was a problem hiding this comment.
I'd say throw an exception here as well to be consistent with the behavior that we have in the other classes.
|
😲 this looks almost perfect, I might even be able to merge this today :) |
|
Do you want to keep the "Advanced" word for now or think of another name... hmmmm |
|
@schmittjoh let me know if it's good to go and I can rebase again. |
|
All I can think of is "Advanced" or "Extra"..... but I still think this should belong in the base classes & interfaces themselves... so perhaps one day we can bump the version and break BC and hope everyone is using 1.1.* (Since this is a feature anyway) |
|
Looks good. Let's go with "Advanced" for now. |
|
Great! |
Advanced metadata implementation.
|
Thanks for your work! |
Fixes #19
Before I head to a holiday work party, I wanted to push what I've got so far, only because I have a few questions pertaining to it.
I was crunching it all together just to get it done real fast.
In this attempt, I was going to make it up to the driver to return the class name, but once I got to the FileDriver, and after looking at the old ones, it looks as if metadata files need to be named according to the class it pertains to (with or without the prefix). Which makes me feel that instead of just returning all files that match for the driver, to instead parse suggested class names from the file name / prefix known to the driver.
What do you think @schmittjoh ?
We can then talk about if we should put a warmupCache method or something in the abstract driver to actually cache metadata and such.
None-the-less, this should be a step in a positive direction as far as this library is concerned I'd say.
I'll add the rest of the tests when I know I'm heading in the right direction. I added one showing the finders functionality before i change it.
Happy holidays!