Skip to content

ResolveBindingsPass: Don't throw error for unused service, missing parent class #27088

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

Merged
merged 1 commit into from
Apr 30, 2018

Conversation

weaverryan
Copy link
Member

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no->
Tests pass? yes
Fixed tickets symfony/flex#346 (comment)
License MIT
Doc PR n/a

Hey guys!

In short: if you:

A) auto-register a class as a service
B) That class's parent class is missing
C) ... but this class/service is unused

Currently, ResolveBindingsPass will fail and throw an exception. The change avoids that - only throwing the exception if the service IS used. This is already done in AutowirePass.

The real issue is DoctrineFixturesBundle, where, on production, the bundle (and so, Fixtures base class) is not installed, causing a build error, even though these service classes are unused.

Cheers!

This mirrors what was already done in AutowirePass
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Good catch

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Apr 30, 2018
@nicolas-grekas
Copy link
Member

Thank you @weaverryan.

@nicolas-grekas nicolas-grekas merged commit 309da92 into symfony:3.4 Apr 30, 2018
nicolas-grekas added a commit that referenced this pull request Apr 30, 2018
… missing parent class (weaverryan)

This PR was merged into the 3.4 branch.

Discussion
----------

ResolveBindingsPass: Don't throw error for unused service, missing parent class

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no->
| Tests pass?   | yes
| Fixed tickets | symfony/flex#346 (comment)
| License       | MIT
| Doc PR        | n/a

Hey guys!

In short: if you:

A) auto-register a class as a service
B) That class's parent class is missing
C) ... but this class/service is unused

Currently, `ResolveBindingsPass` will fail and throw an exception. The change avoids that - only throwing the exception if the service IS used. This is already done in `AutowirePass`.

The real issue is DoctrineFixturesBundle, where, on production, the bundle (and so, `Fixtures` base class) is not installed, causing a build error, even though these service classes are unused.

Cheers!

Commits
-------

309da92 Avoiding an error when an unused service has a missing base class
@weaverryan weaverryan deleted the resolve-bindings-no-error branch April 30, 2018 12:56
This was referenced Apr 30, 2018
@jeroendesloovere
Copy link

Is there any solution out there?
I've posted my environment over here symfony/flex#346 (comment)

@nicolas-grekas
Copy link
Member

@jeroendesloovere looks totally unrelated... If you found a bug, please open a separate issue.

@jeroendesloovere
Copy link

Are you sure?
I have src/DataFixtures/* classes which inherit from DoctrineFixturesBundle which is only loaded in in test and dev environments.

So when I try to deploy to Heroku (which uses prod environment), these DoctrineFixturesBundle classes can't be autoloaded and so everything throws an error.

//...
use Doctrine\Bundle\FixturesBundle\Fixture;
use Doctrine\Common\DataFixtures\OrderedFixtureInterface;
use Doctrine\Common\Persistence\ObjectManager;

class LoadUserData extends Fixture implements OrderedFixtureInterface
{
    public function load(ObjectManager $manager)
    {
        // ...
    }

    public function getOrder()
    {
        return 10;
    }
}

$calls[] = array($constructor, $value->getArguments());
}
} catch (RuntimeException $e) {
$this->container->getDefinition($this->currentId)->addError($e->getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

should be $value->addError(), no ?

Copy link
Member

Choose a reason for hiding this comment

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

this could be an inline definition, but we want the main definition to hold the error

@jeroendesloovere
Copy link

jeroendesloovere commented Jul 6, 2018

Solution

Using composer require doctrine-fixtures-bundle --dev was so wrong.
This has to be composer require doctrine-fixtures-bundle.

Since in my case, this bundle is used in tests.

Wrong composer.json

    "require": {
        // ...
    },
    "require-dev": {
        // ...
        "doctrine/doctrine-fixtures-bundle": "^3.0",
    },

Correct composer.json

    "require": {
        // ...
        "doctrine/doctrine-fixtures-bundle": "^3.0",
    },
    "require-dev": {
        // ...
    },

Other things I tried but failed

# services.yaml
services:

    # ignore datafixtures in production environment,
    # because it usages DoctrineFixturesBundle classes which are only loaded in for "dev" and "test"  environments
    BabyGuesser\DataFixtures\:
        resource: '../src/DataFixtures/*'
        autowire: false
        autoconfigure: false
        public: false

Local everything is perfect, but when pushing to heroku the cache:clear failed every time.

@maximejosien
Copy link

Hello guys,

I had the same error with the DoctrineFixtureBundle, to fix this problem, I added this in my service.yaml

services:

    App\:
        resource: '../src/*'
        exclude: '../src/{DependencyInjection,Entity,Migrations,Tests,DataFixtures,Kernel.php}'

The DateFixtures directory is exclude and I don't have any more this error using prod with this bundle in my composer-dev.

I hope it will help someone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants