Skip to content
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

Failure during install fails to update composer.json file #142

Closed
nicolas-grekas opened this issue Aug 30, 2017 · 13 comments
Closed

Failure during install fails to update composer.json file #142

nicolas-grekas opened this issue Aug 30, 2017 · 13 comments

Comments

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 30, 2017

steps to reproduce

composer create symfony/skeleton foo
cd foo
composer config minimum-stability dev
composer req api

buggy outcome 1/2

Script make cache-warmup returned with error code 2
!!  
!!    [Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException]  
!!    You have requested a non-existent service "".                               

how do you quickly debug that btw? the error message is just useless...

buggy outcome 2/2

composer.json file doesn't contain anything about api-platform, despite the fact the package is installed and listed by composer show.
Do composer up and they are just removed, leaving Flex in an inconsistent state.

@fabpot
Copy link
Member

fabpot commented Aug 30, 2017

Indeed, that's a known issue. The issue here is that if Composer scripts fail, composer reverts the changes done in composer.json. So, a composer up removes the package as it's not listed in composer.json. Not sure what we can do here . Any suggestions?

@fabpot
Copy link
Member

fabpot commented Aug 30, 2017

We could make running the scripts non fatal.

@nicolas-grekas
Copy link
Member Author

Can we hook after composer write the composer.json file?
If yes, can we store the exit status, make the scripts not fail from composer pov, then exit 1?

@nicolas-grekas
Copy link
Member Author

Does composer have any issue already about it? Willing to solve it there? Ping @Seldaek

@chalasr
Copy link
Member

chalasr commented Aug 30, 2017

1/2 is due to api-platform requiring some packages in dev versions, giving a mix of FWB 3.3 and console 3.4/4.0, leading to $container->get(false) because this check is not there in FWB 3.3. We need to either backport the check in FWB:3.3 or add a conflict rule for console>3.4 in FWB:3.3.

@stof
Copy link
Member

stof commented Aug 30, 2017

@nicolas-grekas I don't know of any issue related to this. This would probably involve a new -ignore-script-failure in the require command of composer, to avoid rolling-back the change and the update in case a script fails (allowing you to fix the code and run the command again)

We need to either backport the check in FWB:3.3 or add a conflict rule for console>3.4 in FWB:3.3.

@chalasr having to to this is a sign of a BC break for the console.command.ids parameter in 3.4. We may need to change 3.4 to avoid this instead.

@nicolas-grekas nicolas-grekas changed the title Failure during install fails to update composer.json files Failure during install fails to update composer.json file Aug 30, 2017
@chalasr
Copy link
Member

chalasr commented Aug 31, 2017

@stof you're right, I'll look at it.

fabpot added a commit to symfony/symfony that referenced this issue Sep 8, 2017
…(chalasr)

This PR was merged into the 3.4 branch.

Discussion
----------

[Console] Fix BC break in console.command.ids parameter

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

To make command services lazy loaded when their name is known, we need to exclude them from [runtime registration of other (non lazy-loadable) command services](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L176).
We also need to have them in the `console.command.ids` parameter in order to [not register them by convention](https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/HttpKernel/Bundle/Bundle.php#L188).
It is managed using `false` as values instead of ids for the parameter, [excluding false values from runtime registration](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L177). Problem is that it makes FrameworkBundle's 3.3 Application incompatible with Console 3.4+ given the check for `false` is missing.

This PR fixes it using another parameter referencing ids of command services which can be lazy loaded.
I would be happy to not add the parameter if someone has a suggestion that allows to do so.

Commits
-------

99e95c3 Fix BC break in console.command.ids parameter
symfony-splitter pushed a commit to symfony/console that referenced this issue Sep 8, 2017
…(chalasr)

This PR was merged into the 3.4 branch.

Discussion
----------

[Console] Fix BC break in console.command.ids parameter

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

To make command services lazy loaded when their name is known, we need to exclude them from [runtime registration of other (non lazy-loadable) command services](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L176).
We also need to have them in the `console.command.ids` parameter in order to [not register them by convention](https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/HttpKernel/Bundle/Bundle.php#L188).
It is managed using `false` as values instead of ids for the parameter, [excluding false values from runtime registration](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L177). Problem is that it makes FrameworkBundle's 3.3 Application incompatible with Console 3.4+ given the check for `false` is missing.

This PR fixes it using another parameter referencing ids of command services which can be lazy loaded.
I would be happy to not add the parameter if someone has a suggestion that allows to do so.

Commits
-------

99e95c3d2d Fix BC break in console.command.ids parameter
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this issue Sep 8, 2017
…(chalasr)

This PR was merged into the 3.4 branch.

Discussion
----------

[Console] Fix BC break in console.command.ids parameter

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

To make command services lazy loaded when their name is known, we need to exclude them from [runtime registration of other (non lazy-loadable) command services](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L176).
We also need to have them in the `console.command.ids` parameter in order to [not register them by convention](https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/HttpKernel/Bundle/Bundle.php#L188).
It is managed using `false` as values instead of ids for the parameter, [excluding false values from runtime registration](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Console/Application.php#L177). Problem is that it makes FrameworkBundle's 3.3 Application incompatible with Console 3.4+ given the check for `false` is missing.

This PR fixes it using another parameter referencing ids of command services which can be lazy loaded.
I would be happy to not add the parameter if someone has a suggestion that allows to do so.

Commits
-------

99e95c3d2d Fix BC break in console.command.ids parameter
@weaverryan
Copy link
Member

@Seldaek @naderman Could you guys help? Is there a way to make Composer not revert the composer.json file when a script fails? Or would that need to be added to Composer?

I keep hitting this bad behavior in Flex - it's a major ugly spot :/.

@stof
Copy link
Member

stof commented Nov 24, 2017

We discussed this during the hackday, and they were OK for reverting the composer.json change only for the resolution and installation part, and not for post-update script failures. But @Seldaek said he has not time to work on it soon. So if we want a fix sooner, it is better to provide a PR rather than waiting for him to implement it.

@weaverryan
Copy link
Member

I've just tried something: composer/composer#6842

Do you guys see any issues? If not, could you review/support it?

@weaverryan
Copy link
Member

This should be fixed when Composer 1.5.3 is released: composer/composer#6842 (comment)

We don't print a message at the bottom, but the composer.json file is NOT reverted:

screen shot 2017-11-28 at 4 01 21 pm

I think this is quite fine 👍

@Seldaek
Copy link
Member

Seldaek commented Nov 28, 2017

It looks quite horribly verbose though for a failure that is kinda expected and frequent.. Maybe that could be improved in flex or the cache clear command or both :)

@bitgandtter
Copy link

I think this is a responsibility of the bundle or package to improve the DX and provide a well integrated recipe, in this case DoctrineBundle should provide the version parameter so it avoids the connection and by that the env requirement and by consequence that issue is not presented.

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

No branches or pull requests

7 participants