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

Add IdenticalPropertyNamingStrategy #270

Closed
wants to merge 4 commits into from

Conversation

passkey1510
Copy link

No description provided.

@@ -81,6 +81,7 @@ private function addSerializersSection(NodeBuilder $builder)
->scalarNode('separator')->defaultValue('_')->end()
->booleanNode('lower_case')->defaultTrue()->end()
->booleanNode('enable_cache')->defaultTrue()->end()
->booleanNode('identical')->defaultFalse()->end()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schmittjoh this config for the naming strategy is becoming more and more crappy with the way each new strategy is added as a boolean node. This should probably be refactored

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I made it to false by default for BC reason.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof, strong words :) but I'm also not happy with this development.

@passkey1510
Copy link
Author

Hi, I've made a new pull request to adapt changes in Serializer and SerializerBundle. Thanks.

@schmittjoh
Copy link
Owner

Regarding the configuration, how about something like this:

property_naming:
    id: ~
    strategy: camel_case|identical
    camel_case:
        separator: _
        lower_case: true|false
    enable_annotation: true|false
    enable_cache: true|false

@passkey1510, I'm not implying that you should make the changes (you can though) :) but I can make them when we agree on the general structure.

@passkey1510
Copy link
Author

👍 for introducing strategy option.

@passkey1510
Copy link
Author

But would camel_case option override other strategy defined above. What about:

property_naming:
    id: ~
    strategy:
        camel_case:
            separator: _
            lower_case: true|false
        identical: ~

Then add a check in Configuration so that only one strategy is defined.

@stof
Copy link
Contributor

stof commented Feb 14, 2013

@passkey1510 this is wrong. choosing a strategy is about chossing between camel_case and identical, not about configuring both. Your suggestion has exactly the same issues than the current config.

@passkey1510
Copy link
Author

@stof yes I see what you mean. I did not clarify my proposal but it lists all possible configurations indeed. In fact, strategy section will define what strategy you want to include, so that would be either camel_case or identical (or any custom strategy), but not both. That's why I indicated Configuration should check only one strategy is defined.

It could be rewritten as follow:

property_naming:
    id: ~
    strategy:
        camel_case:
            separator: _
            lower_case: true|false

OR

property_naming:
    id: ~
    strategy:
        identical: ~

@stof
Copy link
Contributor

stof commented Feb 14, 2013

@passkey1510 Which is exactly the same config than the current one (which we find becoming crappy) except that you are adding an extra level.
and btw, the id is also related to the naming strategy. It allows using a custom one by passign a service id.

@passkey1510
Copy link
Author

Ok I see. @schmittjoh any idea?

@ClementGautier
Copy link

I think that these configuration changes have to be discussed in another issue. It may introduce BC breaks depending on the choosen solution and it is not directly related to the subject of this issue. In that way we should be able to merge this feature request.

@stof
Copy link
Contributor

stof commented Feb 15, 2013

@ClementGautier agreed

@ClementGautier
Copy link

@schmittjoh Can you merge this PR ?

@schmittjoh
Copy link
Owner

I agree with @stof here. Could someone implement the config structure that I outlined (and rewrite the existing fields if they are moved to preserve BC)?

@ClementGautier
Copy link

And @stof agreed with me when I said that these changes should be discussed in another PR ...

@schmittjoh
Copy link
Owner

I don't mind doing that, but then we should first do that before we let the structure become more "messy".

If you need a quick fix, please just overwrite the respective service from your app configuration. It should be fairly trivial.

@benwaine
Copy link

@schmittjoh I'm attempting to override the service as you mentioned but the camel case naming strategy is being used. Should be as simple as adding the following to my services.yml file ?

jms_serializer.naming_strategy:
  class: JMS\Serializer\Naming\IdenticalPropertyNamingStrategy

Thanks.

@benwaine
Copy link

For those of you attempting to overwrite the respective service I ended up using a compiler pass to achieve the desired result.

@jdachtera
Copy link

For those who are new to symfony like me and struggeling with snake cased properties here is the complete solution mentioned by @benwaine

Add compiler pass class:

use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;

class SerializerCompilerPass implements CompilerPassInterface{
    public function process(ContainerBuilder $container) {
        $container->setParameter(
            'jms_serializer.cache_naming_strategy.class',
            'JMS\Serializer\Naming\IdenticalPropertyNamingStrategy'
        );
    }
}

Register this compiler pass in your bundle class like this:

class YourBundle extends Bundle {
    public function build(ContainerBuilder $container)
    {
        parent::build($container);
        $container->addCompilerPass(new SerializerCompilerPass());
    }
}

@schmittjoh
Copy link
Owner

A compiler pass is overkill for this.

Just set the parameter directly in your config.yml or any other application config file.

@boomsb
Copy link

boomsb commented Jul 12, 2013

jdachtera: Thanks for the proper way to override these parameters. It also helped me to overload the AnnotationDriver and PropertyMetadata classes to create a new access_type.

@l3l0
Copy link

l3l0 commented Jul 18, 2013

I have simillar issue today, sadly

parameters:
    jms_serializer.naming_strategy.class: JMS\Serializer\Naming\IdenticalPropertyNamingStrategy

simply does not work and I do not want to write custom compiler passes for such simple configuration change ;)

After some debugging I found out that I have CacheNamingStrategy in the container. This strategy delegates stuff into SerializedNameAnnotationStrategy which delegates stuff into CamelCaseNamingStrategy so to use other strategy I need to overwrite class for case_naming_strategy like that:

parameters:
    jms_serializer.camel_case_naming_strategy.class: JMS\Serializer\Naming\IdenticalPropertyNamingStrategy

I hope this will help someone :)

@jdachtera
Copy link

This is what I ended up using:

parameters:
    jms_serializer.cache_naming_strategy.class: JMS\Serializer\Naming\IdenticalPropertyNamingStrategy

@paddycarman
Copy link

@jdachtera: Can you pl. tell me in which file/where I need to add the above lines? Thanks.

@jdachtera
Copy link

@paddycarman: app/config/parameters.yml

@danrot
Copy link

danrot commented Aug 26, 2013

@schmittjoh Is there still some work in progress? This issue looks quite outdated to me, and I am not really glad with overwriting this parameter.

@dunglas
Copy link

dunglas commented Sep 25, 2013

What about this config format:

property_naming:
    id: ~
    strategy:
        name: camel_case|identical
        options:
            separator: _
            lower_case: true|false
    enable_annotation: true|false
    enable_cache: true|false

It allows to have different options for different naming strategy in the future.

@schmittjoh
Copy link
Owner

This is problematic as the options node would have to hold the options for all strategies and people might set options which belong to different strategies at the same time and then wonder why they do not work.

I prefer to have the options for each strategy under a separate configuration key as outlined above.

The real issue here is also not so much about the structure, but more that no-one has spent time implementing it.

@mnapoli
Copy link

mnapoli commented Dec 5, 2013

So what's the status on this?

I couldn't find anything in the docs, what is needed to have camelCase properties not renamed when serialized?

@dunglas
Copy link

dunglas commented Dec 5, 2013

@mnapoli: For now you can put this line in your app/config/config.yml file:

parameters:
    jms_serializer.cache_naming_strategy.class: JMS\Serializer\Naming\IdenticalPropertyNamingStrategy

@mnapoli
Copy link

mnapoli commented Dec 5, 2013

Thanks @dunglas, I've removed this library for now and overloaded FOSRestBundle, too many problems for just unserializing a simple json string.

@odino
Copy link

odino commented Dec 8, 2013

I think there should be an easier way to hack the naming strategy of the whole serializer, currently if you want to implement it at runtime it seems impossible to do so (see #347)

@KeKs0r
Copy link

KeKs0r commented Feb 23, 2014

+1 this is really a must have. I tried to work around this with the cache naming strategy parameter. But then its ignoring manually set serialized names. So I would need to take replace the actual naming strategy with the serializedName strategy and then the identicalproperty strategy as delegate.

Why not just define the service that should be responsible for naming things? Then everybody could mix and match naming strategies and even easily define their own.

I solved it now manually by overriding the bundle alias:

    strego_serializer.cache_strategy:
        class: JMS\Serializer\Naming\CacheNamingStrategy
        arguments:
            - "@jms_serializer.serialized_name_annotation_strategy"

    strego_serializer.serialized_strategy:
        class: JMS\Serializer\Naming\SerializedNameAnnotationStrategy
        arguments:
            - "@jms_serializer.serialized_name_annotation_strategy"

    strego_serializer.identical_strategy:
        class: JMS\Serializer\Naming\IdenticalPropertyNamingStrategy

    jms_serializer.naming_strategy:
        alias: strego_serializer.cache_strategy

gbirke added a commit to gbirke/nvcapp1 that referenced this pull request Mar 29, 2014
Ember expects the field names to be CamelCase.
Change serializer naming strategy to reflect that.

See schmittjoh/JMSSerializerBundle#270 and
schmittjoh/JMSSerializerBundle#139
@awdng
Copy link

awdng commented Apr 22, 2014

I don't like the solution to overwrite the default naming strategy in parameters.yml as that file should only include enviroment specific options (eg. database, other servers).

Overriding the service in config.yml worked for me:

services:
    jms_serializer.cache_naming_strategy:
        class: JMS\Serializer\Naming\IdenticalPropertyNamingStrategy
...

Not sure about the implications of this, works fine for me though. I also think this is badly needed to properly work with REST websevices without needing to manually rewrite field names.

@jrmyio
Copy link

jrmyio commented Jun 27, 2014

Awdng, that solution prevents you from using serialized-name (in for example the virtual property).

@schmittjoh I'm still waiting for a solution....

@maff
Copy link
Contributor

maff commented Apr 2, 2015

See #445 - I solved the identical property/serialized name issue with by defining a custom service which uses SerializedNameAnnotationStrategy with IdenticalPropertyNamingStrategy as delegate.

@goetas goetas added this to the v2.0 milestone Jan 20, 2017
@goetas
Copy link
Collaborator

goetas commented Mar 29, 2017

solved with 4b35af9

@goetas goetas closed this Mar 29, 2017
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