8000 [Config] Extract the definition component from the config component · Issue #32847 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Config] Extract the definition component from the config component #32847

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.

Sign up for GitHub

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

Jump to bottom
Closed
Jelle-S opened this issue Aug 1, 2019 · 7 comments
Closed

[Config] Extract the definition component from the config component #32847

Jelle-S opened this issue Aug 1, 2019 · 7 comments
Labels
Config

Comments

@Jelle-S
Copy link
Jelle-S commented Aug 1, 2019 •
edited
Loading

Coming from https://www.drupal.org/project/drupal/issues/1632930 and having seen #7796 I would like to inform if your stance on this has changed in the meanwhile.

Drupal folks are discussing to basically re-implement the same functionality the Symfony Config component provides (see https://www.drupal.org/project/drupal/issues/3021900 and https://www.drupal.org/project/drupal/issues/3021899#comment-12909285), which seems a bit silly to me. The reason they don't use the config component is because it was deemed too much overhead (https://www.drupal.org/project/drupal/issues/1632930#comment-6646246)

Is this up for debate? And if not, is there anything we can do to possibly change your mind?

TIA!
Kind regards,
Jelle
@xabbuh xabbuh added the Config label Aug 1, 2019
@linaori
Copy link
Contributor
linaori commented Aug 1, 2019

Those links all point to issues/posts that are rather dated (up to 7 years) and I'm not exactly sure what the point of this issue is, can you elaborate?

Is this up for debate? And if not, is there anything we can do to possibly change your mind?

What exactly is up for debate and what should be changed?

@Jelle-S
Copy link
Author
Jelle-S commented Aug 1, 2019 •
edited
Loading

I'm sorry, I should've been more clear.

I'm mainly talking about https://www.drupal.org/project/drupal/issues/3021900 which is a more recent one, where they are talking about implementing the new features introduced in Symfony 3.3. In one of it's sub-issues it was stated that some of these features reside in symfony/config (https://www.drupal.org/project/drupal/issues/3021899#comment-12909285), but it was decided not to use that component because it would be too much overhead (Drupal doesn't need/want to use the classes in the Symfony\Component\Config\Definition namespace).

What exactly is up for debate and what should be changed?

In #7796 fabpot stated it would not be worthwhile splitting the config component into three components (config loading, config definition, config cache). Since that is a decision made in 2015, I was wondering if you still feel that it would not be worthwhile splitting it up. So basically what I'm asking is, if you would reconsider #7796?

@linaori
Copy link
Contributor
linaori commented Aug 1, 2019 •
edited
Loading

What would be the benefit of splitting it up? They are tightly coupled.

The config component is used to parse the bundle config (and services/parameters), validate this and then have Symfony process this configuration to manipulate the container. Taken from #7796, I would like to add some comments.

Configuration definition/validation (in Config/Definition)
This is the "template" it has to work with, like which nodes and which types and which values are allowed. This can be useful on its own, and would basically be a glorified way of defining what an array should look like.

Configuration loading (in Config/Loader)
This is actually specific to processing the previous, this doesn't work without the definition, unless you want to allow a raw data set to enter, which I advice against.

Caching things in a file on disk with a revalidation mechanism (ConfigCache et. al.)
Is this even required? The original resources are already tracked in Symfony and the container will be invalidated in dev mode when changes are made, triggering recompilation of the container.

While splitting the first one off might give a benefit, I don't see why Drupal would need to split this. I don't see the problem with using the config component this way and don't understand what the arguments against using it are about.

taken from https://www.drupal.org/project/drupal/issues/1632930#comment-6646246

I considered this thrice already, but I reviewed the sf Config component's code and it is simply too much.

Who cares about the internal implementation size?

This adds 250+ KB of yaml-to-array-to-treebuilder-to-typeddata-to-validation insanity to Drupal core that, realistically, can only be slow as hell.

250 KB of source isn't a problem, especially not with opcache. Slow? It's only ran when the container needs to compile, so that's not an issue either, besides of that, it's lightning fast.

Please have a look into the deeper directories of the sf Config component. I have absolutely no idea why sf folks decided to make that a single component, given that they have overly abstract things like Validator already, and a seemingly too similar DomCrawler.

I don't think this is relevant to using it or not. If complexity is an issue, I don't think using a framework is the right choice. Besides of that, custom code becomes complex as well.

Iterating over array keys and loading resources takes much less than 10% of this component. No need to even remotely introduce a chance for invoking that insane treebuilder stuff.

I don't get problem in this comment

While this comment is very old by now, it's also incorrect by the way it was assessed. So my question is, why should it be re-considered to split it? They should be used together if you want to compile a container and configure things, unless you want to write your own service loader, which is basically reinventing the wheel for no good purpose.

@Jelle-S
Copy link
Author
Jelle-S commented Aug 1, 2019

@linaori Thank you for taking the time to write such an elaborate reply. I'll try to get more feedback from the Drupal community, linking to your explanation above. You might be right when you say the initial assessment was incorrect. I would need to get feedback from some people that know this part of the Drupal codebase like the back of their hand. I would very much like seeing the Symfony Config component, and all the benefits with it, in Drupal core myself. Maybe the Drupal community/core commiters will reconsider themselves, or reassess the config component.

Thanks again for your time!

@linaori
Copy link
Contributor
linaori commented Aug 1, 2019

I'm not familiar with how Drupal works, so I can't comment on that part. My recommendation to Drupal would be to keep it simple, use the Container (compilation)/config/resource tracking in a way that Symfony does if you use the full framework. This provides the most benefits, though I understand if this is difficult to do for a CMS that doesn't want to rely on have a build step first.

@nicolas-grekas
Copy link
Member

What @linaori said. Closing for now, please reopen/resubmit with details when you have more.

@stof
Copy link
Member
stof commented Sep 9, 2019

@linaori there are some mistakes in what you said. The Config/Loader and Config/Resources layer is independent from the Config/Definition layer.
Config/Definition is a separate thing, and it generally processes the config after it has been loaded (and Symfony uses it only for the semantic config of bundles, not for other places loading config files).

But anyway, I still don't understand the reason for this request here. Are you asking to extract Config/Definition to be able to use that part, or to be able to use the rest of Config (loading config with resource tracking) without that part ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Config
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@nicolas-grekas @stof @linaori @Jelle-S @xabbuh
0