Skip to content
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

Use PHP to define the services and routes of the app #721

Merged
1 commit merged into from
Apr 2, 2020
Merged

Use PHP to define the services and routes of the app #721

1 commit merged into from
Apr 2, 2020

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jan 10, 2020

Q A
License MIT
Doc issue/PR -

This PR is here to demonstrate the new capabilities of Symfony 5.1.
Basically, we can now develop a full Symfony app in a single file, like "microframeworks"
No services.yaml, no routes.yaml here.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

@danabrey
Copy link

There is a lot of documentation and tutorial content that will become confusing when this change is made. SymfonyCasts doesn't generally refer to PHP service/route configuration even being a possibility, every example is based on the .yaml files. I realise these can be updated but hopefully that transition can be made quickly enough to not put off too many newcomers :)

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

@nicolas-grekas
Copy link
Member Author

The validation error from the bot is a bug /cc @fabpot :)

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

@weaverryan
Copy link
Member

There is a lot of documentation and tutorial content that will become confusing when this change is made. SymfonyCasts doesn't generally refer to PHP service/route configuration even being a possibility, every example is based on the .yaml files. I realise these can be updated but hopefully that transition can be made quickly enough to not put off too many newcomers :)

Yea, this is the BIG cost - not to be taken lightly. I think it's at least good to explore though.

In practice, if we were to make a big recipe change like this, it makes more sense on a major version (even if the major version doesn't contain the feature that's required to power the recipe). Then you can say "Oh, in Symfony 6, it's done this new way". For SymfonyCasts, if we made this change for 5.1, then any Symfony 5 screencasts we make between now and May (next 5ish months) will have a big outdated piece. I don't think it's worth it - especially because routes.yaml and services.yaml are not huge YAML pain-points anymore (as you don't do a lot of coding in them, typically thank to autowiring & annotations).

@javiereguiluz
Copy link
Member

Thanks for proposing this feature!

I like it a lot ... but I have a minor comment: I agree with Yonel and Maxime ... I like this approach, but I'd prefer to keep the config/services.php and config/routes.php files.

I think it makes things easier to understand, especially for newcomers, and also solves the problem that would make the Kernel class grow too much (up to thousands of lines) when you have lots of routes or services.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jan 11, 2020

The Kernel holds nothing but configuration. The Kernel is configuration. Look at it.
I get this is surprising, like anything new and unexpected.
Yet the Kernel is the perfect place to do configuration - that's its only purpose actually.

Like I mentioned to @yceruto, the Kernel is actually much better than a random file in a folder, because it has context: the use statements, the type hint of the method, the MicroKernelTrait.

All these are the heart of a better configuration system that works with autocompletion.

@Pierstoval
Copy link
Contributor

I also disagree. If one ends up having a big Kernel, it means that on each HTTP request, this "big kernel" is loaded for no reason (loaded, not executed).

Contrary to services.{yaml,php} which is loaded only at compile-time.

Also, Kernel is documented and explained as the "app encapsulator". App config is mostly in config/.

An alternative to provide such direct example and demonstration would be a special skeleton or generator to create a single-file app with only the Kernel.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

@nicolas-grekas nicolas-grekas marked this pull request as ready for review January 11, 2020 21:24
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

@nicolas-grekas
Copy link
Member Author

I cracked it, the DI+Routes config are now split from the Kernel.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

@Tobion
Copy link
Member

Tobion commented Jan 25, 2020

Considering most people are comfortable using YAML for services and routes, I don't see the need to change the default to PHP. If people want to use this, they can easily do so in their app as this PR shows. But I don't think it's a good idea to make this the new default at this point.

@mnapoli
Copy link

mnapoli commented Jan 26, 2020

This is awesome!

Programming PHP applications with PHP! No, I am not being ironic ^^ For PHP developers not that familiar with Symfony, this is conceptually simpler to use PHP than learning a Symfony-specific DSL written in YAML. A PHP API is familiar and discoverable easily (with any IDE) without having to keep the docs on a second screen.

It makes sense that experienced Symfony developers might not see the point in this (after all, what is the problem to fix here?), but I think it may go a long way for casual Symfony developers or junior developers.

(by "casual" I mean developers, me included, that know the framework but not enough to remember all the details)

❤️ this PR!

@kuraobi
Copy link

kuraobi commented Jan 26, 2020

I think there is more to the debate than just "why do we need YAML anyway?" . I have 2 main concerns with this approach:

  • configuring services through PHP opens the door to... anything, including dirty hacks and weird magic code that developers unfamiliar with Symfony (or lazy, or who believe living only once is an excuse to get away with everything) will inevitably come up with. I don't want anyone to have to work on projects were even the DI and config part of the code is crappy magic code, been there already with Laravel. YAML is not perfect but, just like Twig, it provides an adequately constrained subset of features that allow you to do what you need while forcing you to do it in a proper way. Junior devs don't struggle with YAML, they struggle with DI and there is no shortcut through that, they have to learn it.
  • looks like we are going to have another pseudo-fluent API where you're supposed to indent your method calls to try to keep track of what you're calling them onto. The tree builder is already the most painful part of Symfony to use/read/review/test, I don't think we should replicate the idea anywhere.

@Pierstoval
Copy link
Contributor

I also agree with this feature. It's time to move forward to "proper" conventions!

Yaml has never been a good convention IMO. It's only positive value is the fact that it can be easily read. But indentation, complete lack of documentation and validation, etc. (parsing time? :p ) make it a bad practice at first sight. There's also the fact that newcomers (I've seen that too much since I'm a Symfony teacher) are keen to believe that their Yaml config looks similar when defining extensions configuration, services definitions, parameters, routing, fixtures (using Faker and some fixtures-related libs that use Yaml), ORM mapping, and many other things that we can define in Yaml these days.

Many developers argue that XML is the best choice because of the XSD file that make it documented, auto-completed in ANY code editor (since XML has been here for 22 years and XSD for 16 years).

I'm on the side of the ones that like compromises, and the best compromise to me is to use the native API in PHP. This way, native auto-completion is possible, dynamic loading is possible too (like in Compiler Passes, in the end...), environment-specific config becomes possible in one single file (something impossible in Yaml), etc.

Laravel, in comparison, is one of the most popular frameworks, and there's no sign of Yaml in its default configuration. All of it is plain PHP arrays, which is similar than Yaml in its readability, and some parts are using some OOP like routes. It's not perfect (at all, I said a lot about this in a tweeter feed some time ago), but still, it's PHP, and people are used to it.

The benefits of using PHP are numerous, compared to Yaml which only has one.

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

Some random thought

symfony/framework-bundle/5.1/config/routes.php Outdated Show resolved Hide resolved
symfony/framework-bundle/5.1/config/services.php Outdated Show resolved Hide resolved
symfony/framework-bundle/5.1/config/services.php Outdated Show resolved Hide resolved
symfony/framework-bundle/5.1/config/services.php Outdated Show resolved Hide resolved
symfony/framework-bundle/5.1/src/Kernel.php Show resolved Hide resolved
symfony/framework-bundle/5.1/src/Kernel.php Outdated Show resolved Hide resolved
fabpot added a commit to symfony/symfony that referenced this pull request Jan 30, 2020
…DSL configurators (nicolas-grekas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[DI][Routing] add wither to configure the path of PHP-DSL configurators

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

This makes PHP-DSL configurators more flexible, by allowing to use them for a different path than they were initially created for.

Sidekick of symfony/recipes#721

Commits
-------

8f92c85 [DI][Routing] add wither to configure the path of PHP-DSL configurators
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Comment on lines +49 to +57
if ('test' === $kernel->getEnvironment()) {
// When a test case needs access to a service, getting it via
// a public alias with the "test." prefix is recommended.
$services->public()
// ->alias('test.App\MyService', App\MyService::class)
;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this be a readability issue when projects grow and test overrides become more numerous? Shouldn't we stick to services_test.php as usual?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's why I preferred having everything in the Kernel, as that allowed for using methods to split the code :)

But anyway, this can be added later on, on projects that actually need it.

Copy link
Member

Choose a reason for hiding this comment

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

This section isn't needed anymore, as we removed the services_test.yaml file (there is that test container where everything is public)

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

I like it!

@ghost ghost merged commit ed25f50 into symfony:master Apr 2, 2020
@nicolas-grekas nicolas-grekas deleted the php-dsl branch April 2, 2020 20:11
@nicolas-grekas
Copy link
Member Author

oups :)

@weaverryan
Copy link
Member

weaverryan commented Apr 2, 2020

For the record, while I really like this, I'm -1 for doing it now. A big change like this should be done on a major release - it’s confusing for important files to disappear from 5.0 to 5.1. It also has huge effects on docs and screencasts. That’s manageable if we do it on a major release - but not in a minor release.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet