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] Add support for short services configurators syntax#19190

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

Closed
voronkovich wants to merge4 commits intosymfony:masterfromvoronkovich:improve-configurator-syntax
Closed

[DependencyInjection] Add support for short services configurators syntax#19190

voronkovich wants to merge4 commits intosymfony:masterfromvoronkovich:improve-configurator-syntax

Conversation

@voronkovich
Copy link
Contributor

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
LicenseMIT

This PR adds support for short services configurators syntax in YAML files:

services:app.some_service:class:...# Common syntaxconfigurator:[ '@app.configurator', 'configure' ]# Short syntaxconfigurator:'app.configurator:configure'

linaori reacted with thumbs up emoji
returnarray($this->resolveServices('@'.$parts[0]),$parts[1]);
}

return$callable;
Copy link
Contributor

Choose a reason for hiding this comment

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

elseif can be a simpleif.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@hhamon, You're right, with standaloneif code is more readable. I've changedelseif toif. Thanks!

@ro0NL
Copy link
Contributor

What about@app.configurator:configure? I would prefer keeping the@ as a service indicator.

@nicolas-grekas
Copy link
Member

As far as I understand correctly, this continues#12008 that missed the "configurator" case?
Could you add a few tests please?

@voronkovich
Copy link
ContributorAuthor

@ro0NL, I would prefer to use@ too. But it will be a BC break.

nicolas-grekas reacted with thumbs up emoji

@ro0NL
Copy link
Contributor

ro0NL commentedJun 27, 2016
edited
Loading

I see. Maybe it's good to mention this is a generic format, ie i thought it was configurator specific at first, but it's also a notation we can use on factories.

The BC is due the fact service id's allow for colons? Im not sure how intuitive this (new) format will be..., what about@app.configurator->configure =/ Anyway is this really better than
['@app.configurator', 'configure'] which is pretty short already imo. We gain 5 chars or so...

@voronkovich
Copy link
ContributorAuthor

@ro0NL, it is not a new format. It will be a BC break because some people already use short syntax for service factories in their code without using leading@.

ro0NL reacted with thumbs up emoji

@stof
Copy link
Member

@voronkovich I don't understand what you mean.app.configurator:configure does not work for a configurator today, because it is not a callable

@stof
Copy link
Member

I'm 👍 for making the configurator configuration support the same short syntax than the factory one in Yaml. Being consistent is a good idea

ro0NL reacted with thumbs up emoji

@voronkovich
Copy link
ContributorAuthor

@nicolas-grekas, do we really need additional tests for this? We already have the tests for services factories:https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php#L157

@stof
Copy link
Member

@voronkovich if there is a new supported syntax, we need to have a test ensuring that we don't break this syntax by mistake

@voronkovich
Copy link
ContributorAuthor

@stof, Thanks for the explanation!@nicolas-grekas, I've added the tests and fixed some errors.

@nicolas-grekas
Copy link
Member

👍

@fabpot
Copy link
Member

Thank you@voronkovich.

@fabpotfabpot closed thisJun 29, 2016
fabpot added a commit that referenced this pull requestJun 29, 2016
…onfigurators syntax (voronkovich)This PR was squashed before being merged into the 3.2-dev branch (closes#19190).Discussion----------[DependencyInjection] Add support for short services configurators syntax| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| License       | MITThis PR adds support for short services configurators syntax in YAML files:```yamlservices:    app.some_service:        class: ...        # Common syntax        configurator: [ '@app.configurator', 'configure' ]        # Short syntax        configurator: 'app.configurator:configure'Commits-------da2757f [DependencyInjection] Add support for short services configurators syntax
@voronkovichvoronkovich deleted the improve-configurator-syntax branchJune 29, 2016 11:30
@fabpotfabpot mentioned this pull requestOct 27, 2016
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedMay 20, 2019
edited
Loading

I propose to deprecate this in#31543

nicolas-grekas added a commit that referenced this pull requestMay 22, 2019
This PR was merged into the 4.4-dev branch.Discussion----------[DI] deprecate short callables in yaml| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        |symfony/symfony-docs#11589This deprecates defining factories as `foo:method` instead of `['@foo', 'method']` in yaml.This is confusing syntax and misses the opportunity to raise an error when one does a typo and uses a single colon instead of two.This basically reverts#19190Commits-------f8a04fd [DI] deprecate short callables in yaml
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@voronkovich@ro0NL@nicolas-grekas@stof@fabpot@hhamon@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp