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

allow null for framework.translator.default_path#40744

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
fabpot merged 1 commit intosymfony:4.4fromSimonHeimberg:patch-3
Aug 26, 2021

Conversation

@SimonHeimberg
Copy link
Contributor

@SimonHeimbergSimonHeimberg commentedApr 8, 2021
edited
Loading

Allow to configure framework.translator.default_path to null, as it was until symfony 3.4.x (fix BC compatibility).

QA
Branch?4.4 for bug fixes
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#37111
LicenseMIT
Doc PRno

@ro0NL
Copy link
Contributor

shouldnt we fix it at the Configuration level, given->defaultValue('%kernel.project_dir%/translations')

IIUCcannotBeEmpty is missing.

@SimonHeimberg
Copy link
ContributorAuthor

shouldnt we fix it at the Configuration level, given->defaultValue('%kernel.project_dir%/translations')

IIUCcannotBeEmpty is missing.

I see two options:

  • allow null again, as it was before 3.4.x (not sure about the exact version)
  • prevent null cleanly (Your suggestion is probably cleaner. Failing with a fatal php error as currently is bad.)

I am not sure which way to go. Actually it was a BC break in 3.4.x, which was not noticed quickly. For more details see#37111

@ro0NL
Copy link
Contributor

ro0NL commentedApr 8, 2021
edited
Loading

i think it broke as of 4.x due the added type hint in ContainerBuilder::fileExists

before the inexistence was implicit, eg.if ($container->fileExists($dir = $defaultDir.'/'.$name)) { checks/$name at root level if $defaultDir is null.

So i think allowing explicitnull in config is reasonable and should be handled as such, everywhere. As it preserves BC with 3.4

SimonHeimberg reacted with thumbs up emoji

@SimonHeimberg
Copy link
ContributorAuthor

SimonHeimberg commentedApr 9, 2021
edited
Loading

Should I add a test case checking this config value? Where should I add it? I doubt thatsrc/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php would test enough places 🤔

Allow to configure framework.translator.default_path to null, as it was until symfony 3.4.x (fix BC compatibility).fixessymfony#37111
@SimonHeimbergSimonHeimberg changed the titleclear failure on warmup when translator.default_path is nullallow null for framework.translator.default_pathApr 9, 2021
@fabpot
Copy link
Member

Thank you@SimonHeimberg.

@fabpotfabpot merged commitdd42aec intosymfony:4.4Aug 26, 2021
This was referencedAug 30, 2021
@SimonHeimbergSimonHeimberg deleted the patch-3 branchOctober 13, 2021 19:54
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

@stloydstloydstloyd left review comments

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

@SimonHeimberg@ro0NL@fabpot@stloyd@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp