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

[DependencyInjection][HttpKernel] Introduce build parameters#47680

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
chalasr merged 1 commit intosymfony:6.3fromHeahDude:feat/di-build-parameters-2
Dec 5, 2022

Conversation

@HeahDude
Copy link
Contributor

@HeahDudeHeahDude commentedSep 24, 2022
edited
Loading

QA
Branch?6.3
Bug fix?no
New feature?yes
Deprecations?yes
Tickets~
LicenseMIT
Doc PRTODO

Alternative implementation of#47608.
Add a convention for parameters named with a starting dot to be available at compile-time only:

$containerBuilder->setParameter('foo');// nothing changes$containerBuilder->setParameter('.bar');// will not be dumped

Calling$container->getParameter('.bar') on a compiled container will throw.

chalasr, alamirault, jeremyFreeAgent, and yceruto reacted with thumbs up emoji
@chalasr
Copy link
Member

What if one gets aParameterBag(Interface) injected directly? Should it throw a generic error when trying to access any param starting with a dot?

@HeahDude
Copy link
ContributorAuthor

HeahDude commentedSep 25, 2022
edited
Loading

@chalasr I thought about adding some logic in theFrozenParameterBag.
I didn't because:

  • with the current implementation, when the container is dumped, the bag is created without the build parameters already (seehttps://github.com/symfony/symfony/blob/6.2/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php#L1567)
  • when injectingContainerBagInterface orParameterBagInterface orPsr\Container\ContainerInterface $parameterBag one actually gets the dumped container under the hood, so unless I'm missing something, it's covered already
  • I'm not sure we want to add complexity to handle throwing when the container is compiled but not dumped, waiting for some feedback about this.
    Also it would cause problem when the dumper gets the frozen bag as it would get such exception while trying to dump parameter values.

@chalasr
Copy link
Member

@HeahDude Makes sense, thanks. That could be done later if needed anyways

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment
edited
Loading

Choose a reason for hiding this comment

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

I like that ;)

Here are some notes about the implementation.

The drawback is that parameters are going to be "public" by default, and everybody will need to add a leading dot almost all the time. On the other hand, it's not a big deal if they don't, parameters are cheap, so it's fine to me.

The next step will likely be to find a way to help the community move from public params to private ones. Here is an idea: when both 'foo' and '.foo' exist, we could consider that this means that using 'foo' at runtime is deprecated. This might help bundles adopt private params.

And another step could be to find a way to completely deprecate a parameter. An idea: params starting-and-ending with a~ would mean that:get('foo') would lookup forget('~foo~') and trigger a deprecation. That'd work with private and public params of course.

Either for this PR, or another one, as you want, if you want :)

HeahDude reacted with hooray emoji
chalasr added a commit that referenced this pull requestSep 26, 2022
…s (HeahDude)This PR was merged into the 6.2 branch.Discussion----------[DependencyInjection] Deprecate numeric parameter names| Q             | A| ------------- | ---| Branch?       | 6.2| Bug fix?      | no| New feature?  | no| Deprecations? | yes| Tickets       | ~| License       | MIT| Doc PR        | TODOWhile trying to use `'.' !== $key[0]` instead of `str_starts_with($key, '.')` in#47680, I noticed some tests were failing due to the usage of numeric parameter names in the fixtures.This leads to inconsistent behavior since the following code: `$parameterBag->set(10, 10)`, will first cast the name `10` to string because of the method signature, but will then cast back to integer when using it as an array key inhttps://github.com/symfony/symfony/blob/6.2/src/Symfony/Component/DependencyInjection/ParameterBag/ParameterBag.php#L89.Because Symfony does not use strict types, it can be really tricky.This PR propose to deprecate using such names to be able to properly throw in 7.0.Commits-------3e0050a [DependencyInjection] Deprecate numeric parameter names
@HeahDude
Copy link
ContributorAuthor

The next step will likely be to find a way to help the community move from public params to private ones. Here is an idea: when both 'foo' and '.foo' exist, we could consider that this means that using 'foo' at runtime is deprecated. This might help bundles adopt private params.

And another step could be to find a way to completely deprecate a parameter. An idea: params starting-and-ending with a ~ would mean that: get('foo') would lookup for get('foo') and trigger a deprecation. That'd work with private and public params of course.

Before working on this, my intention was to work on deprecating parameters (https://symfony-devs.slack.com/archives/C8WHX21K7/p1663341791619939), so I definitely agree that this should be the next step for another PR :).

@lyrixx
Copy link
Member

I like this idea too 👍🏼 I proposed that8 years ago 👼🏼

HeahDude and chapterjason reacted with heart emoji

chalasr
chalasr previously approved these changesOct 6, 2022
@nicolas-grekas
Copy link
Member

We're wondering about how to achieve built-time parameterssince years and this is the best and only actionable idea we've had. I like it a lot personnaly: simple and effective.

ogizanagi reacted with thumbs up emoji

@HeahDudeHeahDudeforce-pushed thefeat/di-build-parameters-2 branch 2 times, most recently froma8110a0 to80561dcCompareDecember 5, 2022 11:50
@nicolas-grekas
Copy link
Member

Any other vote @symfony/mergers ? I'd like to merge this before#48469 to prevent merge conflicts.

Copy link
Member

@ycerutoyceruto left a comment

Choose a reason for hiding this comment

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

Nice addition!

@chalasrchalasrforce-pushed thefeat/di-build-parameters-2 branch fromc32f55b toc75dbcaCompareDecember 5, 2022 12:37
@chalasr
Copy link
Member

Thank you@HeahDude.

HeahDude and jeremyFreeAgent reacted with hooray emojijeremyFreeAgent reacted with heart emoji

@chalasrchalasr merged commit6c8f6b3 intosymfony:6.3Dec 5, 2022
@HeahDudeHeahDude deleted the feat/di-build-parameters-2 branchDecember 5, 2022 17:06
nicolas-grekas added a commit that referenced this pull requestDec 13, 2022
This PR was merged into the 6.3 branch.Discussion----------[HttpKernel] fix wrong deprecation message| Q             | A| ------------- | ---| Branch?       | 6.3| Bug fix?      | yes| New feature?  | no| License       | MITIntroduced in#47680Not sure about how to catch the deprecation.Commits-------6bfc18d [HttpKernel] fix wrong deprecation message
@fabpotfabpot mentioned this pull requestMay 1, 2023
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestMay 24, 2023
This PR was merged into the 6.3 branch.Discussion----------Update parameter to compile-time prefixUse the new compile-time only prefix for build parameters.Seesymfony/symfony#47680 for the original feature implementation.Commits-------cf36166 [Performance] Update parameter to compile-time prefix
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@ycerutoycerutoyceruto approved these changes

@chalasrchalasrchalasr approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

6.3

Development

Successfully merging this pull request may close these issues.

9 participants

@HeahDude@chalasr@lyrixx@nicolas-grekas@wouterj@stof@yceruto@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp