[Config] Introduce concept of deprecating nodes #21051
Conversation
| @trigger_error($message, E_USER_DEPRECATED); | ||
|
|
||
| if (null !== $handler) { | ||
| $normalized = $handler($this->remapXml($normalized)); |
There was a problem hiding this comment.
Oh, and on a side note; it was a really weird experience noticing that xml remapping is applied after any normalization.
I totally expected this to be done before normalization. Like normalizing keys...
Maybe we shouldnt do it even here.. for consistency. But it's really convenient :)
|
Re-considered some things;
This makes the behavior more consistent imo. If you only want a notice and preserve nodes you can do that yourself easily. |
|
Im even thinking expression builder now.. What about $array
->deprecate(['foo', 'bar']) // if part (if any given)
->preserve() // optional
->bridge($callable) // then part
->renameTo('new_%s') // then part
->withMessage('...')
->end() |
|
I wonder if this is worth it, isn't it simple enough to do: $node
->beforeNormalization()
->always(function ($v) {
if (isset($v['foo']) {
@trigger_error('Foo is deprecated', E_USER_DEPRECATED);
}
})
->end();? |
|
Yes and no. It's simple, but cumbersome. The strategy is always the same here (bridging old values to new ones, removing the old ones). And im not 100% sure yet, but i believe we should use So imagine, deprecating multiple fields... we need checks like Besides, without #21052 this approach (nor edit: even considering |
|
@GuilhemN updated PR description with an explicit goal, why i think it's important. |
|
I think I'm 👎 here because it adds a feature that is quite easily implemented directly in the code, and that is not needed by userland (well, could be for bundle authors, but that's quite rare anyway.) |
|
Imo. it could help SF a lot, for contributors to do it intuitively themselves, saving time on reviews. And indeed may help other libs. From a pure component perspective it may benefit many. At least, that were my considerations in the first place. |
I prefer that by all means over writing the same type of normalization closures all the time. Deprecations should not be error prone, and bridge strategies should be explicit. We standardized it for DI services as well. |
|
@ro0NL I understand the goal but I agree with @nicolas-grekas, it is too elaborated for its uses. Maybe a simple utility method based on the current features would be enough, without being a nightmare to maintain ? public function deprecate(string[] $keys, string $message = 'Option "%s" is deprecated.')
{
return $this->beforeNormalization()
->always(function ($v) use ($keys) {
foreach ($keys as $key) {
if (array_key_exists($v, $key)) {
@trigger_error(sprintf($message, $key), E_USER_DEPRECATED);
}
}
return $v;
})
->end();
} |
Agree
Here's where we disagree. I think the component could solve a common case here. My project has few deprecations, many projects have many deprecations. Or at least i see a opportunity for the component to provide a standard here. Deprecations in configs are a real issue, and this could aim for better config management and best practices. Now, i dont want to push anything here and i understand the motivation against it. Always happy to close if i cant convince :) |
|
In the hope to save everyones' time, I'm 👎 here :) |
|
I don't see the need for end user projects to deprecate options, so I'm 👎 as well |
|
Well.. then it's 4-4.. making me already less convinced :) Hope the point is clear this is not about adding features, for the sake of it. The case is tedious, and the component goes beyond "end user projects". 👍 |
This introduces an out-the-box solution for deprecating nodes in a configuration tree, avoid doing it user-land and repetitively.
It works like
Child nodes must be referenced once in mulitple deprecations. meaning you cant do
PR Goal
Utilize (fluent interface) the impleme 8000 ntation of
trigger_error($message, E_USER_DEPRECATED). So that users only have to care about business value; bridging the old scenario to the new one. Aiming to provide consistent deprecation messages by default (again, to not bother users with it). And doing it rather fast.The bridging scenario is either a simple case or a complex one, it doesnt differ when doing it with
beforeNormalizationand is not what this PR is about nor aiming to solve. Although simple scenarios can be utilized.