Add legacy class to fix old Form compilation pass #456

nidble · 2017-11-02T09:44:35Z

I am targeting this branch, because this does not break BC.

Closes sonata-project/SonataAdminBundle#4700

Changelog

### Fixed
DependencyInjection/Compiler/FormFactoryCompilerPass.php
DependencyInjection/Compiler/LegacyFormFactoryCompilerPass.php
SonataCoreBundle.php

Subject

Introduced legacy class to mantain compatibility with old sf versions and to provide a solution against deprecation warnings raised because old FormFactoryCompilerPass extends deprecated FormPass.

After this change if the new FormPass class is available, lt will be correctly extended by FormFactoryCompilerPass while LegacyFormFactoryCompilerPass still will mantain BC.

greg0ire · 2017-11-02T10:03:18Z

Please write a changelog directed at end users. Keep in mind that they probably do not care what file or class you changed, but what features you provided / what bugs you fixed. Try to answer the question "What benefit do I get from upgrading?"
More on http://keepachangelog.com

greg0ire · 2017-11-02T10:05:53Z

DependencyInjection/Compiler/FormFactoryCompilerPass.php

@@ -40,17 +41,19 @@ public function process(ContainerBuilder $container)
        }

        // get factories
-        $original = $container->getDefinition('form.extension');
-        parent::process($container);
+        if (version_compare(Kernel::VERSION, '2.8', '<')) {


Please read this: https://symfony.com/doc/current/setup/bundles.html#updating-your-code-to-support-symfony-2-x-and-3-x-at-the-same-time

I used version_compare Kernel::VERSION because:

it's the first approach inside CompilerPass that I've seen in other CoreBundle files.

this service is mainly used in the SonataListFormMappingCommand that through isEnabled checks the symfony version and not the presence of methods...

I think that better rewrite can be made further considering BC behavior from a global perspective...

it's the first approach inside CompilerPass that I've seen in other CoreBundle files.

Mistakes have been made I'm afraid...

No problem :-) this is my first commit and I'm worrying about standard etc. that I've already messed.

thanks for your patience!

:) please read my last comment though, I think it's the most important: #456 (comment)

BTW, we no longer support symfony versions < 2.8 🤔

edit: @greg0ire You mentioned it on other comments, I will read the full discussion before commenting 😄

greg0ire · 2017-11-02T10:07:06Z

DependencyInjection/SonataCoreExtension.php

@@ -82,7 +82,7 @@ public function load(array $configs, ContainerBuilder $container)
        $this->registerFlashTypes($container, $config);
        $container->setParameter('sonata.core.form_type', $config['form_type']);

-        $this->configureFormFactory($container, $config);
+        // $this->configureFormFactory($container, $config);


Please remove commented code

greg0ire · 2017-11-02T10:07:43Z

Please format your commit message according to our rules :

The commit message subject must be less than 50 characters and tell us what you did.
The commit message body should tell us why you did it. It is optional but highly recommended.

Bad example :

Fixed bug #989 by removing call to defraculate()

Good example:

Remove call to defraculate()

Calling this function caused a bug because it interferes 
with calls to getSchmeckles().
Fixes #989

More details in CONTRIBUTING.md

greg0ire · 2017-11-02T10:14:46Z

Can you explain what this does, and how you changed it? If I had to take a guess, I'd say you are disabling form mapping for all versions greater than 2.8 (or is it lower?) However, we do not support versions lower than 2.8 (look at composer.json), so these conditions are useless, right?

nidble · 2017-11-02T11:20:04Z

The hypothesis about my changes is to restrict the correct behavior only to certain sym version. i.e., considering the "client" class SonataListFormMappingCommand, this code must work with sym versions that differ from 3.x. The rest must be immutate.

To accomplish this I can fix my PR with `version_compare(Kernel::VERSION, '3, '!='), what do you think about?
thanks

greg0ire · 2017-11-02T11:24:18Z

What do you mean by "sym"?

greg0ire · 2017-11-02T11:25:59Z

Oh it's sf, ok

greg0ire · 2017-11-02T11:31:07Z

I'm unsure if the limit should be 3.0 or 3.4, and also, I'm unsure what this means for versions that will not have the code inside the "if". Does it mean people will no longer be able to use sonata_type_datetime_picker inside TheirAdmin::configureFormFields ? If yes, that would be a big BC-break, right?

greg0ire · 2017-11-02T11:34:08Z

I re read the error message and see that this is about extending another FormPass class. Since when does this FormPass class exist. IMO we should extend it as soon as it exists.

greg0ire · 2017-11-02T12:50:09Z

It exists since 3.3.0: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/CHANGELOG.md#330

nidble · 2017-11-02T13:38:38Z

Regarding FormPass, actually there are two implementation, the old one is Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\FormPass(1). The new, introduced by me is: Symfony\Component\Form\DependencyInjection\FormPass(2).

The problem is that (1) raise deprecation warning. But if I try to remove build fail...

greg0ire · 2017-11-02T13:41:36Z

A solution might be to have 2 passes too, and use the right one depending on the existence of Symfony\Component\Form\DependencyInjection\FormPass (no kernel version check please)

nidble · 2017-11-04T15:13:53Z

Hi @greg0ire
I worked on this PR and I made two different Pass. By checking existence of Symfony\Component\Form\DependencyInjection\FormPass now I can choose to add "new pass" or the oldest Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\FormPass.

Before to proceed with a PR, whats will be the name of the new compile pass class?

greg0ire · 2017-11-04T15:24:16Z

I'd rather you keep the old name and rename the old compiler pass to LegacyWhateverTheCurrentNameIs

soullivaneuh

The changelog needs an update.

#456 (comment)

greg0ire · 2017-11-04T16:16:21Z

SonataCoreBundle.php

+        }
+        else {
+            $container->addCompilerPass(new Compiler\LegacyFormFactoryCompilerPass());
+        }


This will make StyleCi angry, please use PSR-2. Also, you could use a ternary inside addCompilerPass instead.

greg0ire · 2017-11-04T17:03:07Z

This should help fixing StyleCi:

git fetch --all --prune
git rebase origin/3.x
git push --force

Hoping origin is the main repo here.

nidble · 2017-11-04T17:47:08Z

git fetch --all --prune

Fetching origin

git rebase origin/3.x

Current branch 3.x is up to date.

git push --force

Everything up-to-date

greg0ire · 2017-11-04T17:48:28Z

git remote -vvv?

nidble · 2017-11-04T17:49:42Z

git remote -vvv

origin https://github.com/nidble/SonataCoreBundle.git (fetch)
origin https://github.com/nidble/SonataCoreBundle.git (push)

greg0ire · 2017-11-04T17:51:16Z

git remote rename origin fork
git remote add origin https://github.com/sonata-project/SonataCoreBundle.git

and then try again previous steps

Legacy FormFactoryCompilerPass raise deprecation warnings because it extends deprecated FormPass. Fixes sonata-project/SonataAdminBundle#4700

greg0ire · 2017-11-04T18:00:39Z

StyleCI is green, good job :)

greg0ire · 2017-11-04T18:04:35Z

Travis is failing though, please fix it.

greg0ire · 2017-11-05T18:36:51Z

No response... I'm working on this myself now.

greg0ire · 2017-11-05T19:03:13Z

Ok so here is my understanding of the situation: the sonata core compiler pass listens to the symfony compiler pass, copies the names of all forms and gives it to https://github.com/sonata-project/SonataCoreBundle/blob/3.x/Form/Extension/DependencyInjectionExtension.php#L20-L23 , which allows to use forms "like before". This will probably not work in Symfony 4, and we should stop supporting it.

So here what I think should be done:

add an option to disable this;
trigger a deprecation error if the user does not disable it.

greg0ire · 2017-11-05T19:16:59Z

Looks like that setting already exists: https://github.com/sonata-project/SonataCoreBundle/blob/3.x/DependencyInjection/Configuration.php#L45

nidble · 2017-11-05T19:29:35Z

@greg0ire
sorry but before to answer you, it was more accurate to fully understand if was necessary or not to provide two different test case...

But now, what did you think that default configuration would be false? And what's the best code strategy to make test pass and code done right?

greg0ire · 2017-11-05T19:33:56Z

If you want this make this work, you should find a way to get the array of form type names / form type extensions from the new FormPass (the one from dependency injection, as opposed from the old one, from framework bundle). I haven't looked into it, because I believe people should no longer use this feature. Working on a PR to deprecate it right now.

what did you think that default configuration would be false?

That would be a huge BC break, so definitely not. We should disable this feature if the user uses Symfony 4, BTW.

greg0ire · 2017-11-05T19:34:35Z

See my tweet for how to get rid of the the deprecation warning: https://twitter.com/greg0ire/status/927256894724608000

greg0ire · 2017-11-05T19:51:54Z

Also see #462

nidble · 2017-11-05T20:25:25Z

Make the appropriate changes to this PR, imho involve in two main point:

improve compilerpass to handle sonata.core.form.extension.dependency service
make a proper test case

But doing this, for a service that is used only by SonataListFormMappingCommand when sf !== 3, with the restriction of your PR, for me is a little bit expensive...

greg0ire · 2017-11-05T20:30:04Z

@nidble you do as you feel, if it's too expensive, don't do it: as I said, people can get rid of the deprecation error the hard (but correct way):

sonata_core: 
    form: 
        mapping: 
            enabled: false

Well, at least I hope! It would be great if someone actually tried this on their application and reported back 😅

I don't understand the when sf !== 3 part though.

improve compilerpass to handle sonata.core.form.extension.dependency service

If you want this to get merged, this service should be able to get all existing form types and extensions, as it does with the legacy version.

nidble · 2017-11-05T22:24:40Z

Sure, previously with sf !== 3, I refered at the condition inside SonataListFormMappingCommand that enable this command, the only one class involved in service utilization.

Providing new version that works in all circumstances is not useful if that command only works for before condition https://github.com/sonata-project/SonataCoreBundle/blob/3.x/src/Command/SonataListFormMappingCommand.php#L32

greg0ire · 2017-11-05T22:28:04Z

Ok, let's close this then?

SonataCI · 2017-11-05T22:30:25Z

Could you please rebase your PR and fix merge conflicts?

nidble mentioned this pull request Nov 2, 2017

FormPass is deprecated in Symfony 3.4 sonata-project/SonataAdminBundle#4700

Closed

greg0ire reviewed Nov 2, 2017

View reviewed changes

nidble changed the title ~~Changed form compilation pass and fixed correct class inheritance to support BC~~ Add legacy class to fix old Form compilation pass Nov 4, 2017

soullivaneuh suggested changes Nov 4, 2017

View reviewed changes

greg0ire reviewed Nov 4, 2017

View reviewed changes

Add legacy class to fix old Form compilation pass

0bef233

Legacy FormFactoryCompilerPass raise deprecation warnings because it extends deprecated FormPass. Fixes sonata-project/SonataAdminBundle#4700

greg0ire mentioned this pull request Nov 4, 2017

Test sf4 sonata-project/SonataAdminBundle#4742

Merged

SonataCI added the pending author label Nov 5, 2017

greg0ire closed this Nov 17, 2017

Add legacy class to fix old Form compilation pass #456

Add legacy class to fix old Form compilation pass #456

Conversation

nidble commented Nov 2, 2017 • edited Loading

Changelog

Subject

greg0ire commented Nov 2, 2017

greg0ire Nov 2, 2017

Choose a reason for hiding this comment

nidble Nov 2, 2017

Choose a reason for hiding this comment

greg0ire Nov 2, 2017

Choose a reason for hiding this comment

nidble Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

greg0ire Nov 2, 2017

Choose a reason for hiding this comment

jlamur Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

greg0ire Nov 2, 2017

Choose a reason for hiding this comment

greg0ire commented Nov 2, 2017

greg0ire commented Nov 2, 2017

nidble commented Nov 2, 2017

greg0ire commented Nov 2, 2017

greg0ire commented Nov 2, 2017

greg0ire commented Nov 2, 2017

greg0ire commented Nov 2, 2017

greg0ire commented Nov 2, 2017

nidble commented Nov 2, 2017

greg0ire commented Nov 2, 2017

nidble commented Nov 4, 2017

greg0ire commented Nov 4, 2017

soullivaneuh left a comment

Choose a reason for hiding this comment

greg0ire Nov 4, 2017

Choose a reason for hiding this comment

greg0ire commented Nov 4, 2017

nidble commented Nov 4, 2017

greg0ire commented Nov 4, 2017

nidble commented Nov 4, 2017

greg0ire commented Nov 4, 2017

greg0ire commented Nov 4, 2017

greg0ire commented Nov 4, 2017

greg0ire commented Nov 5, 2017

greg0ire commented Nov 5, 2017

greg0ire commented Nov 5, 2017

nidble commented Nov 5, 2017

greg0ire commented Nov 5, 2017

greg0ire commented Nov 5, 2017

greg0ire commented Nov 5, 2017

nidble commented Nov 5, 2017

greg0ire commented Nov 5, 2017 • edited Loading

nidble commented Nov 5, 2017

greg0ire commented Nov 5, 2017

SonataCI commented Nov 5, 2017

nidble commented Nov 2, 2017 •

edited

Loading

nidble Nov 2, 2017 •

edited

Loading

jlamur Nov 2, 2017 •

edited

Loading

greg0ire commented Nov 5, 2017 •

edited

Loading