Decouple property naming from PropertyCollection

[Spea]

While working on discriminator support, I stumbled upon a bug where it was impossible to force a camelCase name for a property.

So using a class like this:

class MyClass {
    #[SerializedName('myProperty')]
    public string $myProperty;
}

would result in the following error

LogicException : You can not rename my_property into myProperty as it is the same property. Did you miss to handle camelCase properties with PropertyCollection::serializedName?

With the changes I did in this PR, it is now possible to pass different property naming strategies to the different parsers.
Besides fixing the bug at hand, this also provides more freedom when it comes to property collection handling, as you don't have to be aware that the collection itself creates the serialized name, but instead it is up to the caller to decide what serialized name should be used.

[dbu]

thanks for this pull request. seems like a nice cleanup, that static method was indeed rather ugly.

i am slightly worried that when different parsers are given different strategies (e.g. one is forgotten and the default is taken), the parsers would add metadata to different names, leading to confusion. but i don't see a good way around that, i think we just need to document that.

can you please adjust the readme section https://github.com/liip/metadata-parser?tab=readme-ov-file#property-naming-strategy (haha, were we call it strategy when its a global static flag :-P - you actually convert that to the strategy pattern here)

[dbu]

oh, i vaguely remember that we struggled with different representation of the same property.

so your approach is to always use the serialized name in the metadata and never the original name?

[Spea]

so your approach is to always use the serialized name in the metadata and never the original name?

That was actually already the case, the only difference now is, that the serialized name is created directly in the parser so you have more control over it instead of directly in the PropertyCollection

[Spea]

i am slightly worried that when different parsers are given different strategies (e.g. one is forgotten and the default is taken), the parsers would add metadata to different names, leading to confusion

I thought about this too, and I see a few other directions we could go here

• Create an abstract parser class which handles the default strategy and has a separate method for injecting a different one
• Create a separate interface ModelParserNamingStrategyAwareInterface which has one method setPropertyNamingStrategy and check in the Builder class if the current parser implements this interface and call the related method
• Add a second parameter to the parse method where you have to pass the naming strategy

  parse(RawClassMetadata $classMetadata, PropertyNamingStrategyInterface $namingStrategy): void

This means in all cases the Builder class would be the responsible one for handling naming strategies.

Personally I think the third option is better, but also a bit weird in case the parser does not need any naming strategy (like the LiipMetadataAttributeParser)

What do you think?

[dbu]

i like the third option best. yes, we will pass the strategy even when no naming is needed, but there is no real cost involved as the strategy is injected/created only once.
the consistency of the strategy is very important, so being able to guarantee that seems worth it.

[dbu]

do you adjust this PR correspondingly?

are there other things for version 2.x or should i tag the release once we have these changes?

[Spea]

do you adjust this PR correspondingly?

are there other things for version 2.x or should i tag the release once we have these changes?

Yes, I will adjust this PR accordingly. After this, the last thing I want to add is the discriminator support (which is almost done, at least the metadata-parser side). However, I have an upcoming vacation, so I'm not sure if I can finish it before, but I will definitely finish it this month.

[Spea]

do you adjust this PR correspondingly?

Done via 2006b3d - Thanks for your feedback on this 🙇

[dbu]

great, thanks a lot! i did some tweaks with the changelog, will try to accept those changes myself and merge.

i am in no hurry to release version 2, i will wait for the discriminator support. it will be easier if its in a major version, and maybe we end up wanting to do BC breaks.

[dbu]

Suggested change

	* Added a new `PropertyNamingStrategyInterface` which can be used to modify the naming strategy for properties. Previously
	* Replaced `PropertyCollection::useIdenticalNamingStrategy` with the `PropertyNamingStrategyInterface`. To change the default strategy, you can now inject it into the parser instead of changing a static property.

[dbu]

Suggested change

this could be adjusted via the `PropertyCollection::useIdenticalNamingStrategy` method, but as this was a bit error prone, a separate

[dbu]

Suggested change

	interface was introduced. This new interface also comes with two implementations:
	The library provides two implementations for the property naming strategy:

[dbu]

Suggested change

	* `SnakeCasePropertyNamingStrategy`.
	* `SnakeCasePropertyNamingStrategy` (default).

[dbu]

i applied the changelog changes and merged the branch. thanks a lot for this contribution !