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

[FrameworkBundle] Fall back to default configuration in debug:config and consistently resolve parameter values #42368

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Aug 4, 2021

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #41121
License MIT
Doc PR n/a

Brings back determining the default configuration from pull request #30648. Refactors determining the extension config into a helper method to keep cognitive load low with early exits instead of nested ifs.

Additionaly, while reviewing, we noticed that entries with parameters are not resolved correctly if they come from the default config and an application config is existing but they are not overriden. E.g. the session config for the framework would output save_path: %kernel.cache_dir%/sessions instead of the resolved value.

Tested via unit test (obviously) and by manually removing bundle configuration in a Symfony project.

Beware that I do not know much about bundles and internals, I looked that up while implementing this and might have missed better/simpler solutions or edge cases. Please somebody also do another functional test to ensure this is really working as it should.

@carsonbot carsonbot added this to the 4.4 milestone Aug 4, 2021
@herndlm herndlm force-pushed the bugfix/fallback-to-default-config branch from 7c4add2 to 60bb74f Compare August 4, 2021 10:24
@herndlm herndlm marked this pull request as draft August 4, 2021 10:25
@herndlm herndlm force-pushed the bugfix/fallback-to-default-config branch 2 times, most recently from 089b5f0 to b1f46bc Compare August 4, 2021 10:47
@herndlm herndlm marked this pull request as ready for review August 4, 2021 11:08
@herndlm
Copy link
Contributor Author

herndlm commented Aug 4, 2021

The failing test should be unrelated.

@herndlm herndlm force-pushed the bugfix/fallback-to-default-config branch 2 times, most recently from 86c4586 to cb5e728 Compare August 5, 2021 10:42
Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

looks generally OK to me

previously we resolved values (resolveValue) after processing => https://github.com/symfony/symfony/pull/30648/files#diff-6f37907eb60dac18aec41358f427dab3c34f1c3d6d71247506ee52a856a05a2dL91, but im not sure it's still applicable for just default config now.

@herndlm
Copy link
Contributor Author

herndlm commented Aug 5, 2021

looks generally OK to me

previously we resolved values (resolveValue) after processing => https://github.com/symfony/symfony/pull/30648/files#diff-6f37907eb60dac18aec41358f427dab3c34f1c3d6d71247506ee52a856a05a2dL91, but im not sure it's still applicable for just default config now.

Good catch! This is about env vars, right? I'll try to add a test and adapt that.

@ro0NL
Copy link
Contributor

ro0NL commented Aug 5, 2021

This is about env vars, right

yes :D another node with default env would be nice i guess, but AFAIK that's already taken care of due the outer resolveEnvPlaceholders call already

@herndlm herndlm force-pushed the bugfix/fallback-to-default-config branch from cb5e728 to ceb02cc Compare August 5, 2021 19:09
@herndlm
Copy link
Contributor Author

herndlm commented Aug 5, 2021

This is about env vars, right

yes :D another node with default env would be nice i guess, but AFAIK that's already taken care of due the outer resolveEnvPlaceholders call already

I added another node to check that and yes you're right - it's already taken care of

@herndlm herndlm force-pushed the bugfix/fallback-to-default-config branch 2 times, most recently from b80de04 to 8f85856 Compare August 5, 2021 19:22
@herndlm herndlm force-pushed the bugfix/fallback-to-default-config branch from 8f85856 to 2affabe Compare August 5, 2021 20:01
@herndlm herndlm changed the title [FrameworkBundle] Fall back to default configuration in debug:config [FrameworkBundle] Fall back to default configuration in debug:config and consistently resolve parameter values Aug 5, 2021
@herndlm herndlm force-pushed the bugfix/fallback-to-default-config branch 3 times, most recently from ec2b218 to 7f82294 Compare August 10, 2021 19:23
@herndlm
Copy link
Contributor Author

herndlm commented Aug 11, 2021

@ro0NL sorry to bother you again, 3 questions, no worries if you can't answer all of them

  1. After your approval I did another change making the value resolving consistent as you mentioned in [FrameworkBundle] Fall back to default configuration in debug:config and consistently resolve parameter values #42368 (comment), is that fine with you?
  2. Do you think the failing redis test could be related or can I test this somehow? I was sure it's unrelated but it constantly fails
  3. Is the core team regularly checking PRs like this (e.g. via label) or should somebody be tagged? I want to avoid pinging them unnecessarily

@ro0NL
Copy link
Contributor

ro0NL commented Aug 11, 2021

  1. 👌
  2. im 99.99% sure failing redis connection is unrelated, however https://ci.appveyor.com/project/fabpot/symfony/builds/40325532 looks related (outdated fixture perhaps?)
  3. cc @lyrixx :)

@herndlm
Copy link
Contributor Author

herndlm commented Aug 11, 2021

Grr, so apparently one of the values to check has single ticks on Windows but not on Linux xD I started to ignore appveyor already because it randomly failed..
UPDATE: should be fixed
UPDATE2: installed redis extension and started a redis Docker instance and tests were running fine 🤷‍♂️

➜  symfony git:(bugfix/fallback-to-default-config) ✗ php ./phpunit src/Symfony/Bundle/FrameworkBundle
#!/usr/bin/env php
PHPUnit 9.5.7 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing /Users/herndlm/Development/source/git-forks/symfony/src/Symfony/Bundle/FrameworkBundle
.............................................................   61 / 1427 (  4%)
.............................................................  122 / 1427 (  8%)
.............................................................  183 / 1427 ( 12%)
.............................................................  244 / 1427 ( 17%)
.............................................................  305 / 1427 ( 21%)
.............................................................  366 / 1427 ( 25%)
.............................................................  427 / 1427 ( 29%)
.............................................................  488 / 1427 ( 34%)
.............................................................  549 / 1427 ( 38%)
......................................................SS.....  610 / 1427 ( 42%)
.............................................................  671 / 1427 ( 47%)
.............................................................  732 / 1427 ( 51%)
.............................................................  793 / 1427 ( 55%)
.............................................................  854 / 1427 ( 59%)
.............................................................  915 / 1427 ( 64%)
.............................................................  976 / 1427 ( 68%)
............................................................. 1037 / 1427 ( 72%)
............................................................. 1098 / 1427 ( 76%)
............................................................. 1159 / 1427 ( 81%)
............................................................. 1220 / 1427 ( 85%)
............................................................. 1281 / 1427 ( 89%)
............................................................. 1342 / 1427 ( 94%)
............................................................. 1403 / 1427 ( 98%)
........................                                      1427 / 1427 (100%)

Time: 00:54.738, Memory: 240.50 MB

OK, but incomplete, skipped, or risky tests!
Tests: 1427, Assertions: 3427, Skipped: 2.

Legacy deprecation notices (156)
➜  symfony git:(bugfix/fallback-to-default-config) ✗ php -v
PHP 7.4.22 (cli) (built: Jul 29 2021 18:27:37) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Xdebug v3.0.4, Copyright (c) 2002-2021, by Derick Rethans
    with Zend OPcache v7.4.22, Copyright (c), by Zend Technologies
➜  symfony git:(bugfix/fallback-to-default-config) ✗ git rev-parse HEAD
0939cdc44eeff42d04b4817f876494585cbecd87
➜  symfony git:(bugfix/fallback-to-default-config) ✗

should be all good then

@herndlm herndlm force-pushed the bugfix/fallback-to-default-config branch from 7f82294 to 0939cdc Compare August 11, 2021 07:57
@herndlm herndlm force-pushed the bugfix/fallback-to-default-config branch from 0939cdc to aa12784 Compare August 11, 2021 18:27
@herndlm
Copy link
Contributor Author

herndlm commented Aug 13, 2021

@lyrixx what do you think?

@herndlm herndlm force-pushed the bugfix/fallback-to-default-config branch from aa12784 to 82873f3 Compare August 16, 2021 15:13
@lyrixx
Copy link
Member

lyrixx commented Aug 17, 2021

If it works as expected, let's ship it

@herndlm herndlm force-pushed the bugfix/fallback-to-default-config branch 4 times, most recently from 4afcb30 to 6be6168 Compare August 25, 2021 08:06
@herndlm
Copy link
Contributor Author

herndlm commented Aug 25, 2021

Rebased on latest 4.4 to get rid of the appveyor fails. 7.2 and 7.4 failing tests should be unrelated, 7.2 tests were still working a week ago 🤔

Shall this be merged then or are any further adaptions needed/wanted? @ro0NL @lyrixx

@herndlm herndlm force-pushed the bugfix/fallback-to-default-config branch from 6be6168 to 9b6110b Compare August 25, 2021 09:50
@fabpot
Copy link
Member

fabpot commented Aug 26, 2021

Thank you @herndlm.

@fabpot fabpot merged commit b921fe3 into symfony:4.4 Aug 26, 2021
@herndlm herndlm deleted the bugfix/fallback-to-default-config branch August 26, 2021 12:34
This was referenced Aug 30, 2021
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.

5 participants