Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@herndlm
Copy link
Contributor

@herndlmherndlm commentedAug 4, 2021•
edited
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#41121
LicenseMIT
Doc PRn/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 outputsave_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.

ro0NL reacted with thumbs up emoji
@carsonbotcarsonbot added this to the4.4 milestoneAug 4, 2021
@herndlmherndlmforce-pushed thebugfix/fallback-to-default-config branch from7c4add2 to60bb74fCompareAugust 4, 2021 10:24
@herndlmherndlm marked this pull request as draftAugust 4, 2021 10:25
@herndlmherndlmforce-pushed thebugfix/fallback-to-default-config branch 2 times, most recently from089b5f0 tob1f46bcCompareAugust 4, 2021 10:47
@herndlmherndlm marked this pull request as ready for reviewAugust 4, 2021 11:08
@herndlm
Copy link
ContributorAuthor

herndlm commentedAug 4, 2021•
edited
Loading

The failing test should be unrelated.

@herndlmherndlmforce-pushed thebugfix/fallback-to-default-config branch 2 times, most recently from86c4586 tocb5e728CompareAugust 5, 2021 10:42
Copy link
Contributor

@ro0NLro0NL 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
ContributorAuthor

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

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 outerresolveEnvPlaceholders call already

@herndlmherndlmforce-pushed thebugfix/fallback-to-default-config branch fromcb5e728 toceb02ccCompareAugust 5, 2021 19:09
@herndlm
Copy link
ContributorAuthor

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 outerresolveEnvPlaceholders call already

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

@herndlmherndlmforce-pushed thebugfix/fallback-to-default-config branch 2 times, most recently fromb80de04 to8f85856CompareAugust 5, 2021 19:22
@herndlmherndlmforce-pushed thebugfix/fallback-to-default-config branch from8f85856 to2affabeCompareAugust 5, 2021 20:01
@herndlmherndlm 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 valuesAug 5, 2021
@herndlmherndlmforce-pushed thebugfix/fallback-to-default-config branch 3 times, most recently fromec2b218 to7f82294CompareAugust 10, 2021 19:23
@herndlm
Copy link
ContributorAuthor

@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
lyrixx reacted with thumbs up emoji

@ro0NL
Copy link
Contributor

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

@herndlm
Copy link
ContributorAuthor

herndlm commentedAug 11, 2021•
edited
Loading

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 phpPHPUnit 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 MBOK, but incomplete, skipped, or risky tests!Tests: 1427, Assertions: 3427, Skipped: 2.Legacy deprecation notices (156)➜  symfony git:(bugfix/fallback-to-default-config) ✗ php -vPHP 7.4.22 (cli) (built: Jul 29 2021 18:27:37) ( NTS )Copyright (c) The PHP GroupZend 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 HEAD0939cdc44eeff42d04b4817f876494585cbecd87➜  symfony git:(bugfix/fallback-to-default-config) ✗

should be all good then

@herndlmherndlmforce-pushed thebugfix/fallback-to-default-config branch from7f82294 to0939cdcCompareAugust 11, 2021 07:57
@herndlmherndlmforce-pushed thebugfix/fallback-to-default-config branch from0939cdc toaa12784CompareAugust 11, 2021 18:27
@herndlm
Copy link
ContributorAuthor

@lyrixx what do you think?

@herndlmherndlmforce-pushed thebugfix/fallback-to-default-config branch fromaa12784 to82873f3CompareAugust 16, 2021 15:13
@lyrixx
Copy link
Member

If it works as expected, let's ship it

herndlm reacted with thumbs up emoji

@herndlmherndlmforce-pushed thebugfix/fallback-to-default-config branch 4 times, most recently from4afcb30 to6be6168CompareAugust 25, 2021 08:06
@herndlm
Copy link
ContributorAuthor

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

@herndlmherndlmforce-pushed thebugfix/fallback-to-default-config branch from6be6168 to9b6110bCompareAugust 25, 2021 09:50
@fabpot
Copy link
Member

Thank you@herndlm.

herndlm reacted with thumbs up emoji

@fabpotfabpot merged commitb921fe3 intosymfony:4.4Aug 26, 2021
@herndlmherndlm deleted the bugfix/fallback-to-default-config branchAugust 26, 2021 12:34
This was referencedAug 30, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

+1 more reviewer

@ro0NLro0NLro0NL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

5 participants

@herndlm@ro0NL@lyrixx@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp