Conflict with DI extra bundle #4292

Nightbr · 2017-01-26T11:16:30Z

Hi !

I have an issue with the last release of SonataAdminBundle (3.11.*).

Environment

Environment dev
Debug enabled
PHP version 5.6.29-0+deb8u1

Sonata packages

$ composer show sonata-project/*
sonata-project/admin-bundle              3.10.3             The missing Symfony Admin Generator
sonata-project/block-bundle              3.3.0              Symfony SonataBlockBundle
sonata-project/cache                     1.0.7              Cache library
sonata-project/core-bundle               3.2.0              Symfony SonataCoreBundle
sonata-project/datagrid-bundle           dev-master 9c6f293 Symfony SonataDatagridBundle
sonata-project/doctrine-extensions       1.0.2              Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.0.0              Symfony Sonata / Integrate Doctrine...
sonata-project/easy-extends-bundle       2.1.10             Symfony SonataEasyExtendsBundle
sonata-project/exporter                  1.7.0              Lightweight Exporter library
sonata-project/intl-bundle               dev-master d2eab1c Symfony SonataIntlBundle
sonata-project/media-bundle              dev-master master

Symfony packages

$ composer show symfony/*
symfony/assetic-bundle     v2.8.1  Integrates Assetic into Symfony2
symfony/monolog-bundle     v2.12.1 Symfony MonologBundle
symfony/polyfill-apcu      v1.3.0  Symfony polyfill backporting apcu_* functions to lower PHP v...
symfony/polyfill-intl-icu  v1.3.0  Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-mbstring  v1.3.0  Symfony polyfill for the Mbstring extension
symfony/polyfill-php54     v1.3.0  Symfony polyfill backporting some PHP 5.4+ features to lower...
symfony/polyfill-php55     v1.3.0  Symfony polyfill backporting some PHP 5.5+ features to lower...
symfony/polyfill-php56     v1.3.0  Symfony polyfill backporting some PHP 5.6+ features to lower...
symfony/polyfill-php70     v1.3.0  Symfony polyfill backporting some PHP 7.0+ features to lower...
symfony/polyfill-util      v1.3.0  Symfony utilities for portability of PHP codes
symfony/security-acl       v3.0.0  Symfony Security Component - ACL (Access Control List)
symfony/swiftmailer-bundle v2.4.2  Symfony SwiftmailerBundle
symfony/symfony            v2.8.16 The Symfony PHP framework

PHP version

PHP version 5.6.29-0+deb8u1

Subject

With the last release of SonataAdmin, I have some issues with DI Extra Bundle see this issue schmittjoh/JMSDiExtraBundle#266

All services are not found with basic service annotation:

/**
 * @DI\Service("app.indexer.category")
 */
class CategoryIndexer

Also, it could be a conflict with my JMS configuration:

jms_di_extra:
    locations:
        all_bundles: false
        bundles: [AppBundle, ApiBundle]
        directories: ["%kernel.root_dir%/../src"]

I tried to use "jms/di-extra-bundle": "^1.7" but the problem still persists...

My workaround so far: rollback to SonataAdminBundle 3.10.* 😕

Steps to reproduce

Update to SonataAdminBundle 3.11.*
Declare services with JMSDiExtraBundle @di\service()
Try to get the service (dep injection or get from container)

Expected results

All services in AppBundle which uses DI annotation must be found as service.

Actual results

Services declared with DI annotation are not found.

[Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException]                                           
  The service "fos_elastica.listener.app.category" has a dependency on a non-existent service "app.indexer.category".

Any idea ?

Thanks for your amazing work and your help !

If you need anything ask for it ! ;)

The text was updated successfully, but these errors were encountered:

maarekj · 2017-01-26T17:32:58Z

I had the same issue, and I fixed it with this lines in config.yml

jms_di_extra:
   annotation_patterns:
      - "JMS\\DiExtraBundle\\Annotation"

Nightbr · 2017-01-26T17:43:33Z

@maarekj Thanks made the trick! Hope we find a better solution that SonataAdminBundle doesn't break main App.

greg0ire · 2017-01-26T18:42:14Z

Can one of you use git bisect to find the offending commit?

Nightbr · 2017-01-27T07:45:35Z

Hey, the only change related to JMSDIExtra is this one:

216933f#diff-d81fe8cfe0444cbcab9e87a76c5e9970R219

EDIT: here is the problem:

        if (isset($bundles['JMSDiExtraBundle'])) {
            $container->prependExtensionConfig(
                'jms_di_extra',
                array(
                    'annotation_patterns' => array(
                        'Sonata\AdminBundle\Annotation',
                    ),
                )
            );
        }

I think we should test if the default annotation_patterns is set. If not we should add "JMS\\DiExtraBundle\\Annotation" (default annotation_patterns) with 'Sonata\AdminBundle\Annotation'.

Default annotation_patterns in JMS:
https://github.com/schmittjoh/JMSDiExtraBundle/blob/master/DependencyInjection/Configuration.php#L79

greg0ire · 2017-01-27T09:04:26Z

If it is already set, our values should be merged with existing values, right? That sounds like a bug to me…

Nightbr · 2017-01-27T09:17:09Z

Nop it's not a bug but a misunderstanding of the default value.

For example, with this jms_di_extra configuration:

jms_di_extra:
    locations:
        all_bundles: false
        bundles: [AppBundle, ApiBundle]
        directories: ["%kernel.root_dir%/../src"]

When I called prependExtensionConfig my annotation_patterns if empty so the config for annotation_patterns will be only

  'annotation_patterns' => array(
       'Sonata\AdminBundle\Annotation',
 ),

And now, the annotation_patterns config is not empty (contains 'Sonata\AdminBundle\Annotation') so the default value JMS\DiExtraBundle\Annotation is replace by 'Sonata\AdminBundle\Annotation'.

It's the correct behavior of DepInjection configuration.

greg0ire · 2017-01-27T09:23:09Z

Oh ok I get it, but I'm not sure I agree with your fix… because that would mean that any bundle that wants to prepend any configuration would have to do the same.

greg0ire · 2017-01-27T09:23:39Z

Very similar problems : #2425 sonata-project/SonataPageBundle#776 Maybe @core23 is right and there should be a native way to do this.

greg0ire · 2017-01-27T09:25:42Z

Maybe cannotBeOverwritten would be it ? http://symfony.com/doc/current/components/config/definition.html#merging-options

Nightbr · 2017-01-27T09:25:56Z

No problem, I don't say that my fix is ideal ;)
We can discuss here for an ideal fix with pleasure !

greg0ire · 2017-01-27T09:27:33Z

If there is no native way I think your fix might be the best course of action… can you check if cannotBeOverwritten works? I have some doubts, I think it might mean that the first thing to configure the bundle wins, not sure it has anything to do with defaults.

Nightbr · 2017-01-27T09:29:33Z

Seems the cannotBeOverwritten doesn't work with Default value...

greg0ire · 2017-01-27T09:33:30Z

Ping @core23 @gremo, what do you think?

greg0ire · 2017-01-27T09:43:47Z

because that would mean that any bundle that wants to prepend any configuration would have to do the same.

Actually in that case, [the first bundle wins](https://symfony.com/doc/3.1/bundles/prepend_extension.html#more-than-one-bundle-using-
prependextensioninterface)

gremo · 2017-01-27T09:49:18Z

@greg0ire are you sure about? Because while writing and testing sonata-project/SonataUserBundle#829 and sonata-project/SonataClassificationBundle#271 I've found that actually the configuration is merged (controller paths from both bundles get merged and dumped). I'm going to test it right now.

greg0ire · 2017-01-27T09:50:49Z

Are you talking about merging with default values, or merging with existing configuration?

Nightbr · 2017-01-27T10:04:37Z

After some research, I confirm what I said later, the prependExtensionConfig doesn't take into account the defaultValue set in configuration.php.

https://github.com/symfony/dependency-injection/blob/74b310dbf6a5e717e4c8aafcb2e1ba2eab116a52/ContainerBuilder.php#L535

https://github.com/symfony/dependency-injection/blob/93f50bfe952da569c1185c718ef41b61fa9d3461/ParameterBag/EnvPlaceholderParameterBag.php#L42

greg0ire · 2017-01-27T16:23:44Z

So I asked on the symfony slack, and I was told this by @xabbuh :

@greg0ire using array_merge() in the "before normalization“ step should work

Here are the related docs, can you try something like that in the DI bundle?
The only problem I see with this is : what if people don't want to merge?

xabbuh · 2017-01-27T16:37:34Z

The change to the Configuration class would have to be done in the JMSDiExtraBundle where it is defined. However, I am not sure that is intended.

gremo · 2017-01-27T17:07:54Z

I can't check right now (in phone) but I would do the following. Please correct me if I'm wrong.

In prependExtensionConfig just check for annotation_pattern. If key doesn't exist, that would mean we need to prepend the default pattern from JMSDiExtraBundle plus the SonataAdminBundle pattern, this way we don't break the bundle.

If key actualy exists it means that those values come from config.yml and not from defaults. Just prepend the sonata pattern, it's the developer responsibility to add any other pattern (including the default ones).

This is a temporary fix but it's necessary because how symfony and jms do merging/prepending is not going to chance in the near future.

greg0ire · 2017-01-27T17:12:23Z

@gremo that's exactly what @Nightbr proposed I think. I was just wondering if that would be something that would make more sense to fix in the DI bundle, but I'm starting to think there is no way to do that properly, b/c you could not guess if the value comes from Sonata and needs to be merged, or if it comes from the developer, in which case they probably want to get it overwritten.

However, for the other problems you had with other bundles, since it's internal, we could go ahead and use what @xabbuh said (@stof says it should actually take place in the validate() method). See https://symfony-devs.slack.com/messages/support/

gremo · 2017-01-27T17:23:19Z

Ok then, lets see what @Nightbr says. I could be wrong but checking for keys (just the presence/absence) should be enought to guess if values come from the developer. Unfortunatly (or not) I'm on holidays until next week and I can't check :)

greg0ire · 2017-01-27T17:27:27Z

Yeah @Nightbr , I think you can go ahead and make PR Sonata-side…
happy holidays @gremo !

Nightbr · 2017-01-30T08:24:49Z

@greg0ire hey, I will make the PR sonata-side next week, I'm on holidays right now too.

greg0ire · 2017-01-30T10:09:18Z

Happy holiday then!

Padam87 · 2017-01-31T15:26:55Z

Is it a good idea to prepend Sonata\AdminBundle\Annotation?

It has caused extreme slowness in dev mode on my projects. Just tried upgrading, and my dev mode runtime went from 0.8 seconds to 2.6.

greg0ire · 2017-01-31T15:29:05Z

We didn't do it yet…

Padam87 · 2017-01-31T15:32:26Z

216933f#diff-d81fe8cfe0444cbcab9e87a76c5e9970

This commit landed in 3.11, and fixed the prepending.

greg0ire · 2017-01-31T15:33:30Z

Oh you're right, sorry! Are you positive that line causes the drop in performance? What if you comment the block?

Padam87 · 2017-01-31T15:36:52Z

Yep. I've reverted to 3.10, all good. Tried adding the annotation manually, took pics :)

Nightbr · 2017-02-09T16:05:23Z

I think we don't need a deprecation with this because it has never worked. It has been introduced in 3.11.* and directly conflict with JMSDiExtra. We should remove this part and documenting the thing as @andreadelfino said. I can modify my PR if you want.

greg0ire · 2017-02-09T16:11:25Z

I think we don't need a deprecation with this because it has never worked

You mean that no one currently is not configuring these annotations, and expecting it to work? Apparently 216933f fucks some things up for some people, but not for people who are using it exclusively with Sonata, right?

Nightbr · 2017-02-09T16:17:49Z

Yep kind of ^^
216933f inverts the condition and when you have configure JMSDiExtra Bundle it automatically add the configuration (which breaks JMSDiExtra default conf).

At 3.10 => works fine but you have to specify manually the AdminAnnotation in your config.yml
At 3.11 => SonataAdminBundle try to (force) add the AdminAnnotation into config when JMSDiExtra is detected. But there is our issue (with default config).

I think we just need to document this properly and remove the part where SonataAdminBundle try to (force) add the AdminAnnotation.

greg0ire · 2017-02-09T16:20:39Z

So since 3.11, it's fair to say new users could have been successfully using this for Sonata. Removing the auto-config would be a BC-break, and should be treated as such (by throwing a deprecation notice telling people they need to do it manually). Otherwise you get people like @andreasferber, that are not very happy and come here to tell "please don't do this", "please don't do that".

Nightbr · 2017-02-09T16:27:35Z

ok why not create a patch 3.11.n with the deprecation notice and remove this code in 3.12. Fair enough 👌

greg0ire · 2017-02-09T16:28:44Z

Deprecation notices are minor, and removals are major… so… your PR : 3.11.n, the deprecation => 3.12, the removal => 4.0.0

Nightbr · 2017-02-09T16:33:18Z

I don't know if we really should merge my PR... It's not a best practice to modify other bundle config on runtime... Wait and see others though on this subject.

greg0ire · 2017-02-09T16:33:55Z

But we're already doing it, right? Might as well do it a bit better…

greg0ire · 2017-02-09T16:35:40Z

If we don't, people will be stuck with this bug until 4.0, because we don't do BC-breaks (well, we try not to).

Nightbr · 2017-02-09T16:40:36Z

Or we can right now remove the auto-config JMSDi code and finally the code works (without workaround) and I think it's not a BC-breaks. And If you want to use AdminAnnotation, you have to add it into your config.yml (we just need to document this a bit).

greg0ire · 2017-02-09T16:42:45Z

So it boils down to : is this a BC-break?

My thoughts:

new user arrives , uses 3.11
expects DI to work with sonata without configuring anything
updating to 3.12 should work the same, and will not if we remove this behavior

@sonata-project/contributors WDYT? Should this be considered a BC-break or not?

andreasferber · 2017-02-09T16:43:49Z

It's definitely a BC break for users using 3.10 or earlier and updating to 3.11 while using DiExtraBundle or later right now...

Nightbr · 2017-02-09T16:45:51Z

agree with @andreasferber and it was my case and the primary reason I opened the issue. It creates a BC-breaks when updates from 3.10 to 3.11 it breaks all Symfony app with JMSDiExtra setup...

greg0ire · 2017-02-09T16:46:23Z

@andreasferber : yes, it is, don't rub it in. What's the lesson we get from reading your sarcasm? That people do not like BC-breaks. Right now you are proposing to create a second BC-break… What I'm proposing is having the right solution in 3.12, but also keep the "bad but better" solution from #4312 around, but deprecated.

greg0ire · 2017-02-09T16:48:42Z

What you guys don't understand is that the BC-break from 3.10 to 3.11 is done. Now it's the new behavior, and it might work for some people. You're trying to fix your use case, but what about people using the other use case, which, by the way, is the documented one? What about people that did not do the upgrade from 3.10 to 3.11 , but will update from 3.10 or lower to 3.12? They will feel just like you two. Don't do unto others what you don't want others to do unto you.

andreasferber · 2017-02-09T16:50:30Z

The solution from #4312 doesn't work, see my comment there.

Nightbr · 2017-02-09T16:52:20Z

Wait and see what other contributors think about this and take a decision.

andreasferber · 2017-02-09T17:07:06Z

Since it appears right now that someone has to "bite the dust", the question is, what is the larger user base, the ones that just started using Sonata admin with version 3.11 to 3.13 and might be using the Admin annotation, or the ones that use Sonata admin for years and want to update their project to the latest version.

Also please think about the impact it has: "Some users have to add a line to their config.yml when updating admin-bundle" vs. "some users are stuck with version 3.10 until 4.0 is eventually released".

greg0ire · 2017-02-09T17:08:49Z

"some users are stuck with version 3.10 until 4.0 is eventually released".

What?

greg0ire · 2017-02-09T17:10:29Z

What I'm proposing is having the right solution in 3.12, but also keep the "bad but better" solution from #4312 around, but deprecated.

They would have to wait until 3.12… so… a few days?

andreasferber · 2017-02-09T17:43:45Z

"keep the solution from #4312 around" would mean right now, that SonataAdminExtension would keep adding its own annotation_pattern without any way to completely disable it. See my first post, the error I'm getting happens as soon as the Admin annotation gets enabled in any way, even if I use 3.10 and add the pattern manually to config.yml. This means, if I can't completely disable the tampering with annotation_patterns, I'm stuck with 3.10 (which only works because of the broken if).

…uration Preserve the default for the annotation_patterns option of DiExtraBundle, otherwise annotations provided by DiExtraBundle itself break if the project has no custom configuration for annotation_patterns.

andreasferber · 2017-02-09T21:17:16Z

In addition to fixing the general issue with JMSDiExtraBundle, my PR also adds a new config option to disable the whole stuff, which provides a workaround to avoid the performance hit (until JMSDiExtraBundle gets fixed) and helps people with a problem like mine.

andreasferber mentioned this issue Feb 9, 2017

Alternative fix for #4292 (problems with JMSDiExtraBundle interaction) #4320

Merged

greg0ire closed this as completed in #4320 Feb 10, 2017

Nightbr mentioned this issue Feb 27, 2017

Sonata admin bundle 3.13 breaks all services definitions #4349

Closed

adadgio mentioned this issue Feb 27, 2017

All services are broken (dev-master) schmittjoh/JMSDiExtraBundle#266

Closed

borodiliz mentioned this issue Mar 22, 2017

Performance: very high initialization time after updating to >=3.11.0 #4409

Closed

This was referenced Mar 21, 2018

Add missing dependency "nelmio/api-doc-bundle" to composer.json sonata-project/SonataClassificationBundle#258

Closed

ApiDoc in method GroupController::getGroupsAction() never imported sonata-project/SonataUserBundle#876

Closed

Conflict with DI extra bundle #4292

Conflict with DI extra bundle #4292

Comments

Nightbr commented Jan 26, 2017

Environment

Sonata packages

Symfony packages

PHP version

Subject

Steps to reproduce

Expected results

Actual results

maarekj commented Jan 26, 2017

Nightbr commented Jan 26, 2017

greg0ire commented Jan 26, 2017

Nightbr commented Jan 27, 2017 • edited Loading

greg0ire commented Jan 27, 2017

Nightbr commented Jan 27, 2017 • edited by OskarStark Loading

greg0ire commented Jan 27, 2017

greg0ire commented Jan 27, 2017 • edited Loading

greg0ire commented Jan 27, 2017

Nightbr commented Jan 27, 2017

greg0ire commented Jan 27, 2017

Nightbr commented Jan 27, 2017

greg0ire commented Jan 27, 2017

greg0ire commented Jan 27, 2017

gremo commented Jan 27, 2017 • edited Loading

greg0ire commented Jan 27, 2017

Nightbr commented Jan 27, 2017

greg0ire commented Jan 27, 2017 • edited Loading

xabbuh commented Jan 27, 2017

gremo commented Jan 27, 2017 • edited Loading

greg0ire commented Jan 27, 2017 • edited Loading

gremo commented Jan 27, 2017

greg0ire commented Jan 27, 2017 • edited Loading

Nightbr commented Jan 30, 2017

greg0ire commented Jan 30, 2017

Padam87 commented Jan 31, 2017

greg0ire commented Jan 31, 2017

Padam87 commented Jan 31, 2017

greg0ire commented Jan 31, 2017 • edited Loading

Padam87 commented Jan 31, 2017

Nightbr commented Feb 9, 2017

greg0ire commented Feb 9, 2017

Nightbr commented Feb 9, 2017

greg0ire commented Feb 9, 2017 • edited Loading

Nightbr commented Feb 9, 2017

greg0ire commented Feb 9, 2017

Nightbr commented Feb 9, 2017

greg0ire commented Feb 9, 2017

greg0ire commented Feb 9, 2017 • edited Loading

Nightbr commented Feb 9, 2017

greg0ire commented Feb 9, 2017 • edited Loading

andreasferber commented Feb 9, 2017 • edited Loading

Nightbr commented Feb 9, 2017

greg0ire commented Feb 9, 2017 • edited Loading

greg0ire commented Feb 9, 2017 • edited Loading

andreasferber commented Feb 9, 2017

Nightbr commented Feb 9, 2017

andreasferber commented Feb 9, 2017

greg0ire commented Feb 9, 2017

greg0ire commented Feb 9, 2017

andreasferber commented Feb 9, 2017

andreasferber commented Feb 9, 2017

Nightbr commented Jan 27, 2017 •

edited

Loading

Nightbr commented Jan 27, 2017 •

edited by OskarStark

Loading

greg0ire commented Jan 27, 2017 •

edited

Loading

gremo commented Jan 27, 2017 •

edited

Loading

greg0ire commented Jan 27, 2017 •

edited

Loading

gremo commented Jan 27, 2017 •

edited

Loading

greg0ire commented Jan 27, 2017 •

edited

Loading

greg0ire commented Jan 27, 2017 •

edited

Loading

greg0ire commented Jan 31, 2017 •

edited

Loading

greg0ire commented Feb 9, 2017 •

edited

Loading

greg0ire commented Feb 9, 2017 •

edited

Loading

greg0ire commented Feb 9, 2017 •

edited

Loading

andreasferber commented Feb 9, 2017 •

edited

Loading

greg0ire commented Feb 9, 2017 •

edited

Loading

greg0ire commented Feb 9, 2017 •

edited

Loading