Skip to content

[DI] More generic loaders #21062

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

Closed
wants to merge 1 commit into from
Closed

[DI] More generic loaders #21062

wants to merge 1 commit into from

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Dec 27, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no (could introduce some though)
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...
  • Standardizes Config\Loader::import() with ImportingLoaderInterface (new interface)
  • Favors Config\FileLoader::setCurrentResource over setCurrentDir
  • Moves container resource tracking to DI\FileLoader::setCurrentResource fixing nested resource tracking out-of-the box.

If we deprecate setCurrentDir the upgrade path would be;

// before
$loader->setCurrentDir($currentDir);
$loader->import($resource, $format, $ignoreErrors, $currentFile);

// after
$loader->setCurrentResource($currentFile);
$loader->import($resource, $format, $ignoreErrors);

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 27, 2016

Practically ready. Tests for Config\Loader, DI\Loader and Routing\Loader all pass. Diff is super clean.

@nicolas-grekas
Copy link
Member

👎 in the same spirit as my other votes today: help everyone save time on discussion+coding when the use case so thin that we can bike-shade about it.
If you don't agree, please argument your use case in a way people can see the benefit for themselves, in their own projects.

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 28, 2016

Regarding the DI loaders; it just simplifies things (imo.). For SF, for me, for everybody.

Anyway, if it's not the better approach or so.. or something is wrong the diff let me know.

I dont know about other use cases, mine showed this works quirky not intuitive. So i did a PR to improve it (imo.)

@nicolas-grekas
Copy link
Member

@ro0NL this still doesn't answer the "use case" question ("simplifies things" is not a use case, you+we need an incentive for your PRs to be merged). You talk about "other use cases", but you still did not describe yours (standardizing/simplifying/moving/refactoring is not a use case - a use case is something that triggers a "wow, with this, I could do that" to a least a few significant people).
Doing this in the hope to help :)

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 28, 2016

I'm aiming for an as low-level usage of the framework backbones as possible. With little deps and by default best DX/feature set as possible.

If i have #19596 (maybe not a true core feature) + DI array loader i'd be super happy :) and im willing to work on it for that matter but ideally not in userland of course.

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 28, 2016

@nicolas-grekas i feel the need to clarify my intentions, a a few PR's of mine basically relate to the same thing; but i overdid it / went too fast.

Goal is: DI\ArrayLoader
Usecase: Simplify low level configuration, without giving in on sugar syntax.

$container->setDefinition('id', new Definition());
new Reference()
new Alias() // etc. etc.

// vs.

return [
   // ...
   'some' => '@ref',
];
  • This PR would make such an implementation nicer (imo.) as it allow easy resource forwarding, to preserve (file) context. And while at it, cleans up the DI\Loader ecosystem a bit.
// SomeFileLoader
$arrayLoader->setCurrentResource($this->currentResource); // we're basically preserving any cosmetic errors here
return $arrayLoader->load(include (array) $phpFile);

Now, to move on, imo. it's worth it (DI\ArrayLoader) and i have few days this week to work on it.

The bigger picture is to at least let YAML make use of it and perhaps tweak the PHP one in order to allow array returning. Im not sure about JSON and we can always do XML later on, or decide not to.

I truly think it's doable and the better approach.

And of course happy new year already 🥂

@unkind
Copy link
Contributor

unkind commented Dec 28, 2016

Goal is: DI\ArrayLoader

@ro0NL if I get you right, exactly this idea was already rejected in that past: #11954

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 28, 2016

Speaking about wow effects... but, wow :)

So given #11954, #11953 and now me... does that qualify (not saying im significant here :))

wow, with this, I could do that" to a least a few significant people

And for

I'm 👎 as enabling the YAML conventions in an array loader looks like a non-sense to me.

Im not sure we should qualify the implementation as strictly being YAML conventions or so. There's nothing YAML specific to some nested key value pairs, neither is @ref, factory:method, etc. This is DI specific, in it's de-normalized form.

@unkind
Copy link
Contributor

unkind commented Dec 28, 2016

This is DI specific, in it's de-normalized form.

This terminology makes sense in your abstract model of DI configuration. But the question is does this abstraction make sense?

I understand that if you want to have YAML-like PHP array configs, you'd duplicate most of the YAML loader. However, I don't see a problem, I think this kind of duplication is OK, you can assume that you just mimic YAML conventions (PhpArrayThatMimicsYamlLoader). Duplication is not always bad thing and abstraction has its cost.

As for me, I don't like idea of @foo syntax in my PHP configs anymore, I'd rather use objects (reference('foo')).

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 28, 2016

abstract model of DI configuration. But the question is does this abstraction make sense?

Good question. I guess super technically spoken yes, but that's truly my opinion. And actually your comment indeed gave good insight about other perspective. Thanks.

I (personally) would favor the cost of abstraction here, and be just more flexible here for whatever format a user is trying to implement. Including core ones.

Back to

This terminology makes sense in your abstract model of DI configuration. But the question is does this abstraction make sense?

It's the true question. We dont really need strict "model of DI config", it' just that having a generic ArrayLoader makes just as much sense as PhpArrayThatMimicsYamlLoader. I just didnt want to name it ArrayThatMimicsYamlLoader really :)

$ret = $loader->load($resource, $type);
} finally {
unset(self::$loading[$resource]);
$this->setCurrentResource($currentResource);
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong, as it does not reset the object in the previous state (as $currentResource is not set directly in the property here, and may not come from the property either).

Adding 1 more state property in the loader is a bad idea IMO, as state in service objects makes them harder to use.

@stof
Copy link
Member

stof commented Dec 28, 2016

I'm -1 on this. I don't see the benefit of this, and it makes the code more complex by adding more state in the loader.

@fabpot
Copy link
Member

fabpot commented Dec 28, 2016

👎 as well. On a side note, I don't think that adding other formats make sense (at least not in core). Second side note: we don't want to uniform the different formats, quite the contrary, we want to make sure we are using the full expressiveness of each format.

Closing as 3 core team members voted -1.

@fabpot fabpot closed this Dec 28, 2016
@ro0NL ro0NL deleted the di/generic-loaders branch December 28, 2016 17:02
@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 28, 2016

👍 i get the perspective.

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.

6 participants