8p / EightPointsGuzzleBundle

⛽️ Integrates Guzzle 6.x, a PHP HTTP Client, into Symfony
MIT License
440 stars 71 forks source link

Add `symfony/asset` in debug mode #294

Closed rvanlaak closed 4 years ago

rvanlaak commented 4 years ago
Q A
Bug fix yes
New feature no
BC breaks no
Deprecations no
Tests pass yes
Fixed tickets #293
License MIT

fixes #293

gregurco commented 4 years ago

Hello @rvanlaak Thanks for issue and PR, but do you know, that when you install packages, the "require-dev" section from packages are totally ignored. "require-dev" are installed only while you are developing it - so if you clone the bundle and run composer install in the root of bundle folder. Do you think that this change will add assets package in your project? I think there are two options: either to add "symfony/asset" in "require" or to add message that package "symfony/asset" should be installed.

Could you please check and confirm that your PR works or another way is required?

rvanlaak commented 4 years ago

Can you elaborate on what you mean with require-dev packages are being ignored? Afaik dev dependencies only will be ignored when running composer install --no-dev.

After running composer require --dev symfony/asset the error from the screenshot was gone. The message you are proposing wouldn't solve the problem, as the error then still would be there. And, the error as given in the screenshot also is clear on that the package is missing.

gregurco commented 4 years ago

After running composer require --dev symfony/asset the error from the screenshot was gone.

yes, because you installed this package in your project. Now try to remove it and install your fork. You will see, that assets packages is not installed.

Can you elaborate on what you mean with require-dev packages are being ignored?

described here: https://getcomposer.org/doc/04-schema.md#require-dev and here: https://github.com/composer/composer/issues/1763#issuecomment-15825444

gregurco commented 4 years ago

@rvanlaak also check this PR: #257 The discussion was not finished, but there are a lot of links and explanations.

rvanlaak commented 4 years ago

Thank you for pointing to #257 , did read through the discussions and agree this PR should get closed as require-dev indeed is root-only.

A better option would be to remove the sole {{ asset() }} usage this bundle has. What about a PR for that?

https://github.com/8p/EightPointsGuzzleBundle/search?q=asset&unscoped_q=asset

What you'd need is to add symfony/twig-bridge to the userland require-dev section when this bundle gets installed, but I'm not sure how to achieve that. Would it be possible to do that via pre-package-install?

"scripts": {
    "pre-package-install": "composer require --dev symfony/web-profiler-bundle"
},
gregurco commented 4 years ago

I think for user it will be not obvious why 2 packages are installed instead of 1. Also I'm not sure if we can remove usage of assets. Need to check that. I think there is no problem, because you receive this error just when open profiler and message is very descriptive. There is no unexpected crash and user has to waste few hours to understand it. Finally we can add this package to suggest section with message that "install this package, if plan to use profiler". What do you think? You went through this problem recently.

rvanlaak commented 4 years ago

The confusing part in the experience I had is that the profiler already crashes on the initial profiler search page (as the panel already gets required over there). Took me a while to figure out this had to do with this bundle.

As symfony/asset isn't required via web-profiler-bundle already that means it probably is easy to get rid of it 👍 Found out solving this is super simple after checking the way how symfony/web-profiler-bundle does solve this. Will file you another PR to resolve this in a few minutes.

gregurco commented 4 years ago

Great :+1:

rvanlaak commented 4 years ago

see #296 for other attempt on fixing #293