[WIP] Update jms/serializer-bundle configuration #293

covex-nn · 2018-02-15T08:10:39Z

Q	A
License	MIT

With this PR new directory %kernel.project_dir%/config/metadata/jms_serializer will be created, where serialization metadata configuration should be stored for objects with App namespace prefix

ghost

Pull request passes validation.

covex-nn · 2018-02-16T06:13:55Z

@goetas please, review changes

goetas · 2018-02-16T08:19:06Z

From the technical point of view is correct. Since symfony "wiped-out" the AppBundle, this makes sense to me.
But I will still keep the section commented (and keep it just as a suggestion) as most of the people are using the annotations.

So for me is 👎 as it is now.

goetas · 2018-02-16T08:19:47Z

jms/serializer-bundle/2.0/config/packages/jms_serializer.yaml

+        directories:
+            app:
+                namespace_prefix: "App"
+                path: "%kernel.project_dir%/config/serializer"


Is this the recommended directory to put metadata configurations?

Probably the dir name should be jms_serializer, to avoid conflicts with the symfony serializer

There is no recommended directory for such metadata configurations. But there was a discussion about a place of translations directory:

First it was moved to /config/translations (see symfony/recipes#249), but then it was moved back to the root of repository (see symfony/recipes#253) because Discoverability of this directory is important because it's one of those dirs that could be browsed by non backend developers (such as assets/ and templates/). (c)

I've chosen config/serilizer because metadata configuration files are stored in <bundle-directory>/Resources/config/serializer for bundles and discoverabilty of config/serialier directory is not important as for translations, and because metadata configuration is configuration =)

What about %kernel.project_dir%/config/metadata/jms_serializer ?

Sounds good!
Cache dir is %kernel.cache_dir%/jms_serializer and metadata configuration will be in %kernel.project_dir%/config/metadata/jms_serializer. Similar and unique )

Cache dir is %kernel.cache_dir%/jms_serializer and metadata configuration will be in > %kernel.project_dir%/config/metadata/jms_serializer. Similar and unique )

👍

goetas · 2018-02-16T08:20:24Z

jms/serializer-bundle/2.0/config/packages/jms_serializer.yaml

-#                namespace_prefix: "My\\BarBundle"
-#                path: "@MyBarBundle/Resources/config/serializer"
+    metadata:
+        auto_detection: true


do not remember if auto_detection: true and directories work well together

they work together, i checked
see https://github.com/schmittjoh/JMSSerializerBundle/blob/master/DependencyInjection/JMSSerializerExtension.php#L130

great! I forgot about it! :)

ghost

Pull request passes validation.

goetas · 2018-02-16T08:49:29Z

I see you have commented back some part of it, should you comment everything?

auto_detection is true by default, no need to specify it

ghost

Pull request passes validation.

covex-nn · 2018-02-16T09:01:03Z

I thought # auto_detection: false and auto_detection: true is the same. And with auto_detection: true there is only one commented block.

I've commented auto_detection parameter. But i would like to to leave %kernel.project_dir%/config/metadata/jms_serializer directory uncommented to be able to create a recipe with serializer metadata configuration. To make it available out from a box

goetas · 2018-02-16T10:09:56Z

I will comment everything just to not overload the user... If the user is using annotations (by experience most of them do), this section should be commented.

I'm ok with suggesting nice defaults (ready to be un-commented by the user) but the user should not have to do extra operations most of the time.

covex-nn · 2018-02-16T13:56:31Z

I will try to argue one more time (the last one =)

Before Flex we had to create AppBundle in src directory and jms/serializer-bundle anyway would check src/AppBundle/Resources/config/serializer dir because of auto_detection: true. With my proposal a number of directories with metadata configuration is the same. So, it is not a overload, IMO. But if most of users are using annotations, may be you are right and i should comment everything (i am not sure =)

Anyway, i'd like to show you an examle: covex-nn@2e5da5c. This is a commit with recipe for sonata-project/notification-bundle (bundle itself must be released before i will create a PR, and this commit is not last, i guess).

A recipe contains: (1) Bundle configuration, (2) Entity App\Entity\SonataNotificationMessage class, (3) Serializer metadata configuration
Bundle does not require doctrine/doctrine-bundle and jms/serializer-bundle, they are optional

After composer require sonata-project/notification-bundle, entity class will be copied by Flex in src/Entity directory and if DoctrineBundle in already installed, after bin/console doctrine:schema:update, database will be updated. At the moment i cannot use @ JMS\Serializer\AnnotationGroup annotation, because of error:

[Semantical Error] The annotation "@ JMS\Serializer\Annotation\Groups" in class App\Entity\SonataNotificationMessage does not exist, or could not be auto-loaded.

But after composer require jms/serializer-bundle serialization will be available and already set up correctly, because Entity.SonataNotificationMessage.xml was copied from recipe too. That is the reason why i suggest not to comment config/metadata/jms_serializer directory.

If you are still not agree with me, i will comment new directory in configuration.

stof · 2018-02-16T14:11:41Z

We could keep the directory mapping uncommented IMO. If the directory does not exist or does not have mapping in it, JMSSerializerBundle should be smart enough to detect this at build time, and so there won't be any performance impact to have it configured even when using annotations AFAIK.

ghost

Pull request passes validation.

goetas · 2018-02-16T15:33:37Z

Do nor remember if JMSSerializerBundle is smart enough to not trigger errors if the dir does not exists or similar....

covex-nn · 2018-02-16T15:47:17Z

JMSSerializerBundle will throw RuntimeException if directory from jms_serializer.metadata.directories does not exists, thats true. But %kernel.project_dir%/config/metadata/jms_serializer will be created by Flex after a recipe will be applied, so no problems here

goetas · 2018-02-16T16:09:57Z

Then I'm ok with it!
👍

ghost

Pull request passes validation.

covex-nn · 2018-02-18T05:52:44Z

@Nyholm PR is ready and reviewed. Please review or/and accept.

covex-nn · 2018-02-24T21:36:20Z

I've got a error message when i tried to create a recipe for sonata-project/notification-bundle:

sonata-project/notification-bundle/3.4/config/metadata/jms_serializer/Entity.SonataNotificationMessage.xml
Underscore notation is required for file names under config/

So, if metadata directory will be inside config/, then this PR will be useless: it will be imposible to put metadata files there with recipe =( I will move this directory from config/ to the root

ghost

Pull request passes validation.

Nyholm

Excuse my ignorance. But what is metadata? Is it code, configuration or temporary files? Will a developer ever change that metadata?

Nyholm · 2018-02-25T10:38:03Z

jms/serializer-bundle/2.0/manifest.json

@@ -3,6 +3,7 @@
        "JMS\\SerializerBundle\\JMSSerializerBundle": ["all"]
    },
    "copy-from-recipe": {
-        "config/": "%CONFIG_DIR%/"
+        "config/": "%CONFIG_DIR%/",
+        "metadata/": "metadata/"


Will this always work? Don't we need a prefix?

I guess it will always work, because %CONFIG_DIR% === 'config'

covex-nn · 2018-02-25T15:06:31Z

@Nyholm i planned to put metadata/ directory into config/, but files like SonataNotificationMessage.orm.xml and Entity.SonataNotificationMessage.xml did not pass Flex Server validation. And now it is looks like i am trying to use non-standard directory for a popular bundle. This is not right, i guess. I will create an issue to discuss it.

ghost

Pull request passes validation.

covex-nn · 2018-02-25T15:53:22Z

@Nyholm metadata is configuration, that describes data for some purposes. Metadata configuration for jms/serializer-bundle could be described via annotations, but sometimes JMS Serializer is optional and it is not possible to use that annotations. It cases like #302 metadata configuration must be stored somewhere in external files. That files could be updated by developer

Nyholm · 2018-02-25T16:09:35Z

Since “metadata” in this case actually is config, I would put it in the config dir. Let’s investigate in the issue with camelCase or snake_case.

Good that you opened that issue.

covex-nn · 2018-04-25T20:21:47Z

Closing PR, as we found another way

ghost deployed to staging February 15, 2018 08:10 View deployment

ghost approved these changes Feb 15, 2018

View reviewed changes

goetas reviewed Feb 16, 2018

View reviewed changes

ghost deployed to staging February 16, 2018 08:46 View deployment

ghost approved these changes Feb 16, 2018

View reviewed changes

ghost deployed to staging February 16, 2018 08:57 View deployment

ghost approved these changes Feb 16, 2018

View reviewed changes

covex-nn mentioned this pull request Feb 16, 2018

[RFC] Improve namespace for flex sonata-project/SonataEasyExtendsBundle#172

Closed

ghost deployed to staging February 16, 2018 15:18 View deployment

ghost approved these changes Feb 16, 2018

View reviewed changes

goetas approved these changes Feb 16, 2018

View reviewed changes

covex-nn closed this Feb 18, 2018

covex-nn reopened this Feb 18, 2018

ghost deployed to staging February 18, 2018 05:26 View deployment

ghost approved these changes Feb 18, 2018

View reviewed changes

covex-nn added 2 commits February 25, 2018 00:41

Update jms/serializer-bundle configuration

03b20aa

Move metadata directory from config/ to the root

5429ac8

covex-nn force-pushed the jms-serializer branch from 43c8223 to 5429ac8 Compare February 24, 2018 21:42

ghost deployed to staging February 24, 2018 21:42 View deployment

ghost approved these changes Feb 24, 2018

View reviewed changes

covex-nn mentioned this pull request Feb 24, 2018

Add sonata-project/notification-bundle recipe #302

Merged

Nyholm reviewed Feb 25, 2018

View reviewed changes

covex-nn changed the title ~~Update jms/serializer-bundle configuration~~ [WIP] Update jms/serializer-bundle configuration Feb 25, 2018

ghost deployed to staging February 25, 2018 15:06 View deployment

ghost approved these changes Feb 25, 2018

View reviewed changes

covex-nn mentioned this pull request Feb 25, 2018

[RFC] Directory name for metadata configuration #303

Closed

covex-nn closed this Apr 25, 2018

[WIP] Update jms/serializer-bundle configuration #293

[WIP] Update jms/serializer-bundle configuration #293

Conversation

covex-nn commented Feb 15, 2018 • edited

ghost left a comment

Choose a reason for hiding this comment

covex-nn commented Feb 16, 2018

goetas commented Feb 16, 2018 • edited

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

ghost left a comment

Choose a reason for hiding this comment

goetas commented Feb 16, 2018

ghost left a comment

Choose a reason for hiding this comment

covex-nn commented Feb 16, 2018 • edited

goetas commented Feb 16, 2018

covex-nn commented Feb 16, 2018

stof commented Feb 16, 2018

ghost left a comment

Choose a reason for hiding this comment

goetas commented Feb 16, 2018

covex-nn commented Feb 16, 2018 • edited

goetas commented Feb 16, 2018

ghost left a comment

Choose a reason for hiding this comment

covex-nn commented Feb 18, 2018

covex-nn commented Feb 24, 2018

ghost left a comment

Choose a reason for hiding this comment

Nyholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

covex-nn commented Feb 25, 2018

ghost left a comment

Choose a reason for hiding this comment

covex-nn commented Feb 25, 2018

Nyholm commented Feb 25, 2018

covex-nn commented Apr 25, 2018

covex-nn commented Feb 15, 2018 •

edited

goetas commented Feb 16, 2018 •

edited

covex-nn commented Feb 16, 2018 •

edited

covex-nn commented Feb 16, 2018 •

edited