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

Support *.yaml extension #1077

Closed
scaytrase opened this issue Apr 29, 2019 · 13 comments · Fixed by #1078
Closed

Support *.yaml extension #1077

scaytrase opened this issue Apr 29, 2019 · 13 comments · Fixed by #1078

Comments

@scaytrase
Copy link
Contributor

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no

*.yml extension is currently hardcoded. https://github.com/schmittjoh/serializer/blob/master/src/Metadata/Driver/YamlDriver.php#L290

It would be nice to support both, since *.yaml is considered canonical https://yaml.org/faq.html

@scaytrase
Copy link
Contributor Author

@goetas if you have ideas how to implement it without heavy refactoring - we can take the implementation part.

My idea is, as soon this is the protected method - is to create some kind of supported extensions method by default utilizing exsisting one as the only element of array and update (or use another) file driver here https://github.com/schmittjoh/metadata/blob/master/src/Driver/AbstractFileDriver.php#L28 to use multiple extensions

btw, random question. is there any sense in metadata library living outisde the serializer library itself?

@goetas
Copy link
Collaborator

goetas commented Apr 29, 2019

@goetas if you have ideas how to implement it without heavy refactoring - we can take the implementation part.

My idea is, as soon this is the protected method - is to create some kind of supported extensions method by default utilizing exsisting one as the only element of array and update (or use another) file driver here https://github.com/schmittjoh/metadata/blob/master/src/Driver/AbstractFileDriver.php#L28 to use multiple extensions

My idea here is just to crate another metadata diver (as example YmlDriver) and add it to the driver chain, The implementation YmlDriver can just of and forward all the method calls to YamlDriver except of getExtension

btw, random question. is there any sense in metadata library living outisde the serializer library itself?

There are plenty of other libs that use jms/metadata as independent package for other propuses rather that serializations (see https://packagist.org/packages/jms/metadata/dependents), so yes, it makes sense

@scaytrase
Copy link
Contributor Author

My idea here is just to crate another metadata diver (as example YmlDriver) and add it to the driver chain, > The implementation YmlDriver can just of and forward all the method calls to YamlDriver except of getExtension

What about inheritance? As soon as YamlDriver isn't final itsef it could be already extended by users and placing another driver will made them repeat this inheritance, I think.
Also YamlDriver already uses yml extension, so how we name the new class?

Your idea is very simple and I though of it too beforehand. This could be implemented easy.

@goetas
Copy link
Collaborator

goetas commented Apr 30, 2019

I'm a strong believer of composition over inherentance, so unless you have a particular reason for inheritance here, I would prefer composition.

Regarding the naming, to not break things, I think we should keep YamlDriver as it is and add YalDriver (it is inconsistent, but I have no better ideas).

@scaytrase
Copy link
Contributor Author

scaytrase commented Apr 30, 2019

I'm a strong believer of composition over inherentance, so unless you have a particular reason for inheritance here, I would prefer composition.

Me too. but I'm talking about other existing users who may inherit it already. This drivers have three layers of inheritance, so they are not so easy to hook up with composition for now.

YalDriver

Looks strange. How about YamlFIleDriver and YamlDriver extends YmlFileDriver? or something like this? This would be inconsistent only with other file drivers (i.e XmlDriver) but would have correct naming at the end

@goetas
Copy link
Collaborator

goetas commented Apr 30, 2019

Building the drivers using extension will be:

$yaml = new YamlDriver($fileLocator, $this->propertyNamingStrategy, $this->typeParser, $this->expressionEvaluator);

$yml = new YmlFileDriver($fileLocator, $this->propertyNamingStrategy, $this->typeParser, $this->expressionEvaluator);

versus using composition:

$yaml = new YamlDriver($fileLocator, $this->propertyNamingStrategy, $this->typeParser, $this->expressionEvaluator);

$yml = new YmlFileDriver($yaml);

Do not see advantages on inheritance yet...

and the new YmlFileDriver class does not need to inherit from AbstractFileDriver, can just implement the original interface, and proxy everything to $yaml

@scaytrase
Copy link
Contributor Author

scaytrase commented Apr 30, 2019

function returning the file extension is protected. how do you override it with composition? Because public interface does not use the file or extension at all, so we can't just decorate it

@goetas
Copy link
Collaborator

goetas commented Apr 30, 2019

@scaytrase
Copy link
Contributor Author

Well, we can decorate the locator to make it ignore the given extension and forcibly use *.yaml but this looks like some kind of unobvious hack for later library devepment.

But if we do this, we can also implement universal YAML file loader which tries to load both yaml or yml, whatever it finds first and this will be more interesting.

@goetas
Copy link
Collaborator

goetas commented Apr 30, 2019

What about simply overriding loadMetadataForClass in YamlFileDriver and trying to load also .yml files?

This will be ever better and will not require at all to create YmlFileDriver

@scaytrase
Copy link
Contributor Author

scaytrase commented Apr 30, 2019

Sound nice, but $locator is private in AbstractFileDriver so we have to duplicate-store it into YamlDriver. And almost duplicate the contents of loadMetadataForClass and getAllClassNames to ensure yaml are loaded too. This means that it's easier to discard AbstractFileDriver inheritance at all and directly impement AdvancedDriverInterface in YamlDriver keeping protected methods for BC. WDYT?

@goetas
Copy link
Collaborator

goetas commented Apr 30, 2019

Sound nice, but $locator is private in AbstractFileDriver so we have to duplicate-store it into YamlDriver. And almost duplicate the contents of loadMetadataForClass and getAllClassNames to ensure yaml are loaded too. This means that it's easier to discard AbstractFileDriver inheritance at all and directly impement AdvancedDriverInterface in YamlDriver keeping protected methods for BC. WDYT?

Sounds good to me

@scaytrase
Copy link
Contributor Author

#1078 here it is

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

Successfully merging a pull request may close this issue.

2 participants