Skip to content

[DI] Renamed some PHP-DSL functions #36800

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

Merged
merged 1 commit into from
May 16, 2020

Conversation

javiereguiluz
Copy link
Member

@javiereguiluz javiereguiluz commented May 13, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

As discussed in #36778, Symfony wants to move from XML to PHP for its own configuration. I propose these function renames to make the PHP-DSL a bit easier to understand:

<?php
// Before
$services->set(Foo::class)
    ->args([ref(Bar::class), service('stdClass')]);

// After
$services->set(Foo::class)
    ->args([service(Bar::class), inline_service('stdClass')]);

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I'm sorry I'm unable to have a strong opinion on this. Both ways have merits.

Does anyone have some rationale to share that could help decide here?

@javiereguiluz
Copy link
Member Author

My rationale was that when looking at this:

->args([ref(Bar::class), service('stdClass')])

Without reading Symfony Docs, you can understand it as follows:

  • I'm injecting "ref", which sounds like a "reference" but I don't know exactly what refers to (a parameter, a service, an alias, a factory, etc.)
  • I'm injecting a service called stdClass

Both assumptions are wrong ... so you must read the docs and learn about this.


However, to me at least, the proposed methods are self-explanatory and they can be understood without reading or learning anything:

->args([service(Bar::class), inline_service('stdClass')])

OK, inject a service and then inline a service.

(you still need to read the docs to learn everything about those methods ... but the learning curve has softened dramatically in the second case).

@Nyholm
Copy link
Member

Nyholm commented May 13, 2020

I am (currently) not a big fan of PHP configuration. Probably because I never used it. =)

So, with my fresh eyes, using service() instead of ref() seams like an excellent idea because it is clear and understandable.

@sstok
Copy link
Contributor

sstok commented May 13, 2020

My biggest concern is BC compatibility 😛 inline_service() sounds good to me, but references have been used for a long time.

@jderusse
Copy link
Member

Actually I like short names in this config file: arg instead of argument, expr, instead of expression, ref instead of reference.

I'm downvoting because:

  • it's not a service definition, but a reference to an existing service (there's never any ambiguity when I use it). In a XML configuration <service is synonym of register a new Service. That would be confusing.
  • inline has been renamed into service for consistency with yaml, If we renamed it into inline_service then we are no more consistent, and at this end we simply renamed inline into inline_service: a longer method name for no real benefit.

nb: @Nyholm you should have a try.. It's much more convenient than xml/yaml configuration. Autocompletion just work with className, constant, etc, You can leverage language by using loop/if and more... it just work

@weaverryan
Copy link
Member

Hi!

3 things from me:

  1. Creating an inline service is not common, at least not compared to configuring an argument to a service. So its DX shouldn't take precedence.

  2. Calling something a "Reference" internally and using a different name here is not ideal. But it's also not a reference in XML: it's an <argument type="service" id="foo">, a bit consistent with ->arg(service('foo')).

  3. I agree with Javier that this is easier to teach. It would sound like this:

We want to pass the Foo::class service as an argument. Ok, ->arg(Foo::class). Hmm, but that would pass the string class name. How can we tell Symfony that we actually want to pass the Foo::class service? Use the service() helper: ->arg(service(Foo::class)).

So, I like it :)

@GromNaN
Copy link
Member

GromNaN commented May 13, 2020

Do you update blog posts like this when such changes are made ?
https://symfony.com/blog/new-in-symfony-3-4-php-based-configuration-for-services-and-routes

@stof
Copy link
Member

stof commented May 13, 2020

@GromNaN no. We would do a new blog post. The new feature of 3.4 does not change when we refactor it in 5.1. 3.4 still has the old names.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

<argument type="service" id="foo">

that one convinced me!

@nicolas-grekas
Copy link
Member

Thank you @javiereguiluz.

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.

10 participants