Added configuration for default context #556

edefimov · 2017-03-21T20:47:39Z

This PR adds functionality for setting up default context parameters for serializer via configuration:

jms_serializer:
  default_context:
    serialization:
      serialize_null: false
      version: ~ 
      attributes: {}
      groups: ['Default']
    deserialization:
      serialize_null: false
      version: ~ 
      attributes: {}
      groups: ['Default']

Also this PR fixes deprecation warnings in tests.

goetas · 2017-03-21T21:01:24Z

Thanks a lot for the PR, will review this week. And possibly add it to the next release

goetas

Great feature!

Sorry for being picky in the review, let me know what do you think about it?

goetas · 2017-03-22T08:27:15Z

DependencyInjection/Configuration.php

+    private function addContextSection(NodeBuilder $builder)
+    {
+        $builder
+            ->arrayNode('context')


you should name this differently since we have both, "serialization" and "deserialization" context.

I suggest default_serialization_context. What do you think?

Are you going to implement it also for the default_deserialization_context?

I supposed to use the same context for both serialization and deserialization purposes. But the idea with separate contexts is, of course, more flexible. So yes, I think I'll split them.

If you like you can also create default_context, default_serialization_context and default_serialization_context. Where is possible to define only default_context. Up to you

goetas · 2017-03-22T08:30:38Z

DependencyInjection/Configuration.php

+                                    throw new InvalidArgumentException('Version must be a string.');
+                                }
+
+                                if (!preg_match('#\d+\.\d+\.\d+#', $value)) {


I will avoid this extra regex... just becase version schemes change often.

In a project in the past I've worked, versions were just one digit.

Under the hood comparison uses version_compare php function. Ok, we can avoid this, but some version string could have no meaning.

I saw, but I prefer to not force a versioning scheme by design. prefer to leave it, so if version_compare changes behavior, do not have to update configurations

goetas · 2017-03-22T08:45:08Z

DependencyInjection/Configuration.php

+        $builder
+            ->arrayNode('context')
+                ->addDefaultsIfNotSet()
+                ->children()


what about having also just id, to specify just a service id as context factory? and when specified, the rest of options are just ignored
It will be super flexible.

I understand this is an extra requirement that can be implemented later, so this is just a suggestion...

goetas · 2017-03-22T08:45:52Z

DependencyInjection/JMSSerializerExtension.php

-                if (!$rs = @mkdir($dir, 0777, true)) {
-                    throw new RuntimeException(sprintf('Could not create cache directory "%s".', $dir));
-                }
+            if (!is_dir($dir) && !@mkdir($dir, 0777, true) && !is_dir($dir)) {


goetas · 2017-03-22T08:47:45Z

Resources/config/services.xml

@@ -185,10 +184,10 @@
            <argument type="constant">NULL</argument> <!-- expression evaluator -->

            <call method="setSerializationContextFactory">
-                <argument type="service" id="jms_serializer.serialization_context_factory" />
+                <argument type="service" id="jms_serializer.context_factory" />


I will keep here two duplicate instances... having the same class, but two different services.

serialization_context_factory and deserialization_context_factory

What for? This will save couple of bytes in ram.

you will need it since you have serialization context and deserialization context (that might have different configurations), you will need two different instances

goetas · 2017-03-22T08:51:13Z

ContextFactory/ConfiguredContextFactory.php

+     *
+     * @param array $config Context configuration
+     */
+    public function __construct(array $config)


never liked "magic" arrays of configurations... to buggy and you always have to read the implementation to know what happens.

why not put into this class properties as groups, version, serialize_nulls..., with getters and setters, and configure them via dependency injection component?

Not a problem. I've got used to write some "magic" configuration arrays with reference to actual configuration. And hiding this array inside the constructor we guarantee that nothing could change actual values from outside.

a factory is meant to be configured. what should not change is the context.
you can configure the service via JMSSerializerExtension

goetas · 2017-03-23T14:59:43Z

@edefimov What do you think about changes in edefimov#1 ?

I have implemented:

service reference by id,
context default are set by the serializer, not by the bundle logic.
removed multi argument constructor adding some setters on the factory

Edefimov context config

edefimov · 2017-03-27T19:21:45Z

@goetas , could you check this item, it seems to me, that in validation section the correct check is interface_exists, not the class_exists

goetas · 2017-03-28T09:06:45Z

~~why ddc7e85 ? can not understand its propouse and touches many different things in the same comment~~
got it

fixed test

goetas · 2017-03-28T10:35:03Z

Will have a closer look in the next days, but looks ready to be merged. Thanks!

goetas reviewed Mar 22, 2017

View reviewed changes

goetas added enhancement need work labels Mar 22, 2017

goetas self-assigned this Mar 22, 2017

goetas added this to the v1.4 milestone Mar 24, 2017

edefimov and others added 6 commits March 27, 2017 22:11

Added configuration for default context, small refactoring

6944a4c

Keep context default out of the bundle logic

e82a99b

allow to specify service id as context factory

178f2b8

Split context configuration to serialization and deserialization part

b1adf5e

Merge pull request #1 from goetas/edefimov-context-config

55baef0

Edefimov context config

Added normalization for "id" configuration

ddc7e85

edefimov force-pushed the context-config branch from ab8dadf to ddc7e85 Compare March 27, 2017 19:19

goetas and others added 2 commits March 28, 2017 11:29

fixed test

c0d3281

Merge pull request #2 from goetas/edefimov-context-config

f24d2e0

fixed test

goetas added to merge and removed need work labels Mar 28, 2017

goetas merged commit 5f7a708 into schmittjoh:master Mar 29, 2017

goetas added a commit that referenced this pull request Apr 10, 2017

Added default_context documentation implemented in #556

90ae10e

iisisrael mentioned this pull request Sep 3, 2017

If no group is defined on SerializationContext all fields with exposed true for groups are exposed #491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added configuration for default context #556

Added configuration for default context #556

edefimov commented Mar 21, 2017 •

edited

Loading

goetas commented Mar 21, 2017

goetas left a comment

goetas Mar 22, 2017

goetas Mar 22, 2017

edefimov Mar 22, 2017

goetas Mar 22, 2017

goetas Mar 22, 2017

edefimov Mar 22, 2017

goetas Mar 22, 2017

goetas Mar 22, 2017

goetas Mar 22, 2017

goetas Mar 22, 2017

edefimov Mar 22, 2017

goetas Mar 22, 2017

goetas Mar 22, 2017 •

edited

Loading

edefimov Mar 22, 2017

goetas Mar 22, 2017

goetas commented Mar 23, 2017 •

edited

Loading

edefimov commented Mar 27, 2017

goetas commented Mar 28, 2017 •

edited

Loading

goetas commented Mar 28, 2017

Added configuration for default context #556

Added configuration for default context #556

Conversation

edefimov commented Mar 21, 2017 • edited Loading

goetas commented Mar 21, 2017

goetas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas commented Mar 23, 2017 • edited Loading

edefimov commented Mar 27, 2017

goetas commented Mar 28, 2017 • edited Loading

goetas commented Mar 28, 2017

edefimov commented Mar 21, 2017 •

edited

Loading

goetas Mar 22, 2017 •

edited

Loading

goetas commented Mar 23, 2017 •

edited

Loading

goetas commented Mar 28, 2017 •

edited

Loading