Skip to content

Use PHP instead of XML as the prefered service/route configuration in core #36778

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
Jun 10, 2020

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented May 10, 2020

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

In Symfony 6, I (we?) would want to promote usage of configuration written in PHP instead of YAML. For third-party bundles and core, we should do the same, replacing XML with PHP. Doing so would remove the need for the XML lib for core.

The biggest advantage is auto-completion with any modern IDE without explicit support for Symfony, and probably one less thing to learn (how to configure things in YAML/XML).

Be warned that it does NOT concern semantic configuration which is much harder.

This PR does the work for the Twig bundle as an example. Once we agreed on the CS, the same should then be done for other core bundles both for service configuration and route configuration.

Embeds #36775 until it is merged.

@fabpot fabpot changed the title Rip xml Use PHP instead of XML as the prefered service/route configuration in core May 10, 2020
@nicolas-grekas nicolas-grekas added this to the next milestone May 10, 2020
@architech99
Copy link

architech99 commented May 10, 2020

Will we sign configuration values from environment variables using getenv() or a super-global like $_SERVER or $_ENV? I think getenv() would be preferred. Also, is the expectation that each configuration file would contain an associative array?

Edit: I jumped on this too quick. The updates posted as I was commenting answer my questions.

->tag('container.preload', ['class' => 'Twig\Template'])
->tag('container.preload', ['class' => 'Twig\TemplateWrapper'])

->alias('Twig_Environment', 'twig')
Copy link
Contributor

@Tobion Tobion May 10, 2020

Choose a reason for hiding this comment

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

pretty strange to me that this is not bound to the service that was defined before. can't we make e.g. $container->services()->set('twig', Environment::class)->alias('Twig_Environment') possible?

Copy link
Member

Choose a reason for hiding this comment

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

This would be based on no factual ground: aliases are standalone things, suggestion something else would create confusion eventually to me.

Copy link
Contributor

@Tobion Tobion Jun 10, 2020

Choose a reason for hiding this comment

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

Sure it should be possible to create aliases standalone. But in most cases you will add an alias on a service you just created like in this example here. Then it makes sense to me not to repeat the service id again avoiding typos. And it reads much better.

Copy link
Member

Choose a reason for hiding this comment

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

But this would remove any possibility to configure the alias (ServiceConfigurator()->alias() returns a configurator for the alias, but in your proposal, we would need to stay in the service configuration).

Also, it cannot be called ->alias(), as that method name is already taken to start a new alias.

And to me, making the PHP DSL define aliases from the service configuration could introduce confusion about how aliases work (and that would not be consistent with other configuration formats btw).


->set('data_collector.twig', TwigDataCollector::class)
->args([ref('twig.profile'), ref('twig')])
->tag('data_collector', ['template' => '@WebProfiler/Collector/twig.html.twig', 'id' => 'twig', 'priority' => 257])
Copy link
Contributor

@Tobion Tobion May 10, 2020

Choose a reason for hiding this comment

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

An idea: we could create an interface for tags as classes.

interface ContainerTagInterface
{
    public function getName(): string;
    public function getAttributes(): array;
}

then allow something like ->tag((new DataCollectorTag)->template('...')->id('twig')->priority(257)) which makes tags discovorable, autocomplete, supporting types and documentable in code.

Copy link
Contributor

@ogizanagi ogizanagi May 10, 2020

Choose a reason for hiding this comment

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

For ref, discoverable tags as constants for usage in the PHP config were discussed a bit in #24377

@fabpot fabpot force-pushed the rip-xml branch 2 times, most recently from 196118f to ad3c202 Compare May 16, 2020 09:14
nicolas-grekas added a commit that referenced this pull request May 16, 2020
This PR was squashed before being merged into the 5.1-dev branch.

Discussion
----------

[DI] Renamed some PHP-DSL functions

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

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

Commits
-------

366405b [DI] Renamed some PHP-DSL functions
@fabpot
Copy link
Member Author

fabpot commented Jun 10, 2020

PR rebased and ready for merge.

@fabpot fabpot force-pushed the rip-xml branch 5 times, most recently from 238fe2b to 5e0d2e9 Compare June 10, 2020 03:20
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.

Good to me, that's a nice move!

$files = Finder::create()->files()->name('*.php')->path('Resources')->notPath('Tests')->in(\dirname(__DIR__, 5));
foreach ($files as $file) {
$contents = file_get_contents($file);
if (preg_match_all("{->tag\('([^']+)'}", $contents, $matches)) {
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with what is done for XML files, should we also look for tagged iterator arguments ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, will do when I have an example :)

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.