| Gábor Hojtsy (he/him) |
Proposed by @Björn Brala (bbrala)i've seen 2 instances recently where changes to .yml give issues. One by berdir with removal of default_url in views, one with definition of new api for categories: https://www.drupal.org/node/3375748How can we support those and make those detectable. |
| Gábor Hojtsy (he/him) |
Are we specifically talking about plugin YAMLs or YAMLs in general (eg. config too)? (edited) |
| Gábor Hojtsy (he/him) |
I think plugin YAMLs are relatively easy because they would have a simpler structure, so the methods used in potx to address them could be reused? |
| Gábor Hojtsy (he/him) |
See https://git.drupalcode.org/project/potx/-/blob/7.x-3.x/yaml_translation_... |
| Gábor Hojtsy (he/him) |
(not exactly the same format of course, but a similar simple solution may work) |
| mikelutz (he/him) |
Plugin yamls are easy because the yaml is passed to a class as config, which can detect deprecated keys in the constructor and issue a deprecation notice. |
| mikelutz (he/him) |
The harder one is configuration yamls. We change the structure, run an update hook to manage the change on live sites, but a module that bundles config has no idea that their config is now invalid. Finishing config validation will help here, as we could have tools to run bundled config in modules against the validation, and they would get an error against new versions of core where the validation rules have changed. |
| Gábor Hojtsy (he/him) |
@mikelutz (he/him) re plugins, I think "the bot" will not instantiate all the plugins probably, although upgrade status can try to instantiate all the plugins somehow and catch deprecation errors, hm |
| Gábor Hojtsy (he/him) |
it will be simpler/faster if we would have a quick static way to detect the deprecated keys |
| mikelutz (he/him) |
Well, that's true, I'm assuming you have testing in your module to cover your yaml plugins. If you are limiting yourself to static analysis without unit tests, then that does get trickier. |
| Björn Brala (bbrala) |
In the linked issue a config key was removed. And if a module had config it broke.Seems like if we are to remove config Keys that break if in config it would be nive to be able to anlyze that. |
| Björn Brala (bbrala) |
Ow wow, sorry my comment was written before your conversation :sweat_smile: |
| Gábor Hojtsy (he/him) |
@Björn Brala (bbrala) we may be able to use parts of the config schema API? (edited) |
| Björn Brala (bbrala) |
Yeah tests should preferably be used. It's about bundled config, so I guess config validation would at some point make validation possible. |
| Björn Brala (bbrala) |
We could probably? But still needs quite some work.#3427641: [meta] Config validation for a more reliable Drupal + reliable Recipes from the start |
| Gábor Hojtsy (he/him) |
No I mean specifically use the config schema system to define the deprecated keys in either core or in upgrade status |
| Gábor Hojtsy (he/him) |
If we need it done ASAP, that is in the Drupal 10 to 11 upgrade path, we need it in upgrade status probably, not in core. |
| Björn Brala (bbrala) |
I think the impact is relatively low, but i can imagine it being a nuisance for module developers. For that specifc annotation it would've been easier when its an attribute i think, since those should just go through phpstan. It wouldn't have been an issue then im thinking.So perhaps we can just ignore this for now, and next cycle we'll have attributes for at least this part. |
| Björn Brala (bbrala) |
I'll keep an extra eye out on the change records to see if any other like this pop up. |
| xjm |
OK weird, I had thought we'd already introduced deprecatibility for all these APIs (also I just coined the word "deprecatibility") |
| xjm |
Oh except config -- which, yes, validation is the answer there IMO |
| Björn Brala (bbrala) |
In the linked change record is is changing the value type of a field in an annotation. |
| Björn Brala (bbrala) |
This is the thread that spawned this thread: https://drupal.slack.com/archives/C03L6441E1W/p1716293607661759 |
| Gábor Hojtsy (he/him) |
@xjm problem with relying on validation is we check for deprecated APIs in the current major for the future major, and the current major should consider the config valid... does config validation have a deprecation category / can be made to throw deprecation exceptions? |
| xjm |
@Gábor Hojtsy (he/him) Well config being data means it should have upgrade paths when we tighten validation, but yes I see what you mean |
| xjm |
For D11, I think we might need to handle all this in the PHP layer, since we are already in beta |
| xjm |
The deprecation APIs we introduced for yaml for things like services and libraries and so on were beta-blocker requirements for the respective major versions (edited) |
| Björn Brala (bbrala) |
so basically, no way out right now, but something to consider for 11. :x |
| xjm |
Yep, I would add it to the D12 roadmap |
| Gábor Hojtsy (he/him) |
hm, how does default shipped config get resolved by an upgrade path? |
| xjm |
We are talking about different things -- I think each needs its own thread |
| xjm |
Plugins and annotation/attribute values are one thing, config schemata another, config data itself yet another, etc. |
| xjm |
So maybe the two problems identified so far should each get their own thread for now, and we add more if we find other regressions |
| xjm |
Also I think the two problems above should get core issues if they don't already have them, be tagged contrib blockers, and be added to the D11 beta2 meta |
| Björn Brala (bbrala) |
i'll try and make/search for the core issues this week. |
| Björn Brala (bbrala) |
and tag them |
| xjm |
@Gábor Hojtsy (he/him) WRT default config, theoretically it gets installed when the module is installed, so we could ahve a PHP/runtime deprecation warning for that as a workaround |
| xjm |
Since modules are installed each time in each test, I mean |
| xjm |
It's not as nice as having static analysis directly on the config, of course, but maybe that combined with upgrade status support for weirder/bad cases might help? |
| Gábor Hojtsy (he/him) |
not all default config is installed 🙂 |
| xjm |
Well it's all installed under some circumstances |
| xjm |
Like optional config is based on another module being installed... are there any cases when it's not? Hmm I haven't thought about that since like... Drupal 8... |
| xjm |
Actually I have no idea how contrib tests that |
| xjm |
OK yeah, tricky |
| xjm |
If it's a core module it's easy to test -- if the contrib module has tests -- but if it's another contrib module that's even less reliable |
| xjm |
Do we actually have any cases in D11 where default config is a problem though? Sorry I lost track of what the second issue was |
| xjm |
https://www.drupal.org/node/2442603 was linked but that is pre-8.0.0 so I don't understand |
| Björn Brala (bbrala) |
Ignore the second issue in that link. |
| Björn Brala (bbrala) |
Was mostly triggered by the first. Which is annotation/attribute |
| Björn Brala (bbrala) |
The other issue was in views where default_url was removed. |
| Björn Brala (bbrala) |
Let me try and find it. |
| Björn Brala (bbrala) |
https://www.drupal.org/node/3382316 |
| Björn Brala (bbrala) |
So that was mentioned by eh, i think @berdir? That after campatibility fixes that came up. |
| Björn Brala (bbrala) |
https://drupal.slack.com/archives/C03L6441E1W/p1716200019951969 |
| xjm |
Ohh I vaguely remember talking about that at DDD |
| xjm |
Well I am glad I asked them to at least make a CR. :joy: |
| Björn Brala (bbrala) |
haha :smile: |
| xjm |
@Gábor Hojtsy (he/him) Can we have one thread for the plugin issue and another for the views issue? |
| xjm |
I want to go into details on both |
| xjm |
@Gábor Hojtsy (he/him) NM Björn has us covered |
| xjm |
Sorry didn't notice how late it's gotten |
| Gábor Hojtsy (he/him) |
proposed by @mikelutz (he/him) #3108658: Handling update path divergence between 11.x and 10.x |
| mikelutz (he/him) |
Need some guidance on where to put effort here, how to handle drush, whether we could convert the service call in the old major to attributes, or if we should move the responsibility of turning the later hook into a noop into the hook itself rather than the updater, since drush has its own, and there could be other custom implementations. |
| xjm |
I would STRONGLY encourage us to go with "minimum possible change" for the issue and descope rearchitecting it better-er to a followup |
| xjm |
Also wow this issue has like 5000 comments I haven't read just in the past week |
| xjm |
@catch’s feedback on the new approach would be a big help here |
| mikelutz (he/him) |
There’s more discussion here: https://drupal.slack.com/archives/C1BMUQ9U6/p1716720512074899 |
| mikelutz (he/him) |
But going over Alex’ approach, I realized it means we have to rewrite the runner in drush too, and set up composer requirements such that your drush version matches the core version of the updater. |
| xjm |
I can't speak to Drush, but yes I'd assume it'd need a followup. Which is another reason to get this in before beta2 in the next few weeks. |
| mikelutz (he/him) |
Which leads me to the next part of this: minimum possible change would be to do nothing to core, and keep it all inside the update hooks. With no change to core at all, you would backport 11001 to 10301 and add \Drupal::keyValue->get('updates_to_skip')->set(11001, TRUE); to the 10301 and wrap all the code in 11001 with if (!\Drupal::keyValue->get('updates_to_skip')->get(11001, FALSE) { … } |
| mikelutz (he/him) |
It’s a pain DX, but would guarantee the future version wouldn’t do anything regardless of the runner used. |
| xjm |
That is... a very good approach actually. Onus on committers to not let anyone eff it up, but that's true in general. :joy: |
| xjm |
I like that because it's easy to understand and we already have similar patterns in core where we've done conditional updates based on the state of the site |
| mikelutz (he/him) |
right, and you wrap the 11 code if there’s any chance of backporting at all. if you never backport, then no big deal. |
| xjm |
The existing API is yucky and sometimes brittle but it does have the advantage of being the devil we know in terms of edgecases |
| mikelutz (he/him) |
And even then, if you can make the hook idempotent, you wouldn’t even need that. if the hook is something like “If A exists, move A into B” then it’s fine to just let it run again, as the next time, A won’t exist so it already won’t do anything. |
| xjm |
if statements wrapping update hooks' function body is already something we should do more of; it makes them more resilient (edited) |
| mikelutz (he/him) |
Right, :joy: I got a chance to explore it while trying my approach. :joy::joy:. I now want to burn it down and rebuild it sometime in the 11.x cycle. But not here. |
| catch |
The trouble with keeping the logic in the update hooks is it creates a chicken and egg between the 10-11 updates. Unless we synchronise commits all the time |
| xjm |
Well in @mikelutz (he/him)’s example the D11 hook only needs to know its own number |
| xjm |
Only the D10 version needs to prognosticate |
| mikelutz (he/him) |
Well, the other issue is it doesn’t deal with the whole issue of the hook being in 11.1, but not 11.0 |
| mikelutz (he/him) |
But I’m still of the opinion we should find a way require 10.4 to go to 11.1 and 10.5 to go to 11.2 as a whole, though it’s tricker than it sounds. |
| mikelutz (he/him) |
Then again, the combination of the two might be easiest. Go with Alex’ service addition, and the check for missing updates in system.install, but just check update_hook_registry->getUpdateSkipped() in the future update hook instead of messing with update_do_one. Then we don’t have to deal with drush, and the future update hook won’t do anything regardless of how it gets executed. IT’s a minimal change to core, just to add some helpers, but doesn’t change anything with the update api at all, still keeps all the runtime responsibility in the hooks themselves. |
| mikelutz (he/him) |
Add the check in d11 if there’s any chance of backporting and even then only if the hook itself is not idempotent and would break something if it ran twice. |
| Björn Brala (bbrala) |
https://www.drupal.org/node/3382316 |
| Björn Brala (bbrala) |
Split from 3️⃣ |
| Björn Brala (bbrala) |
Original source: https://drupal.slack.com/archives/C03L6441E1W/p1716200019951969 (edited) |
| xjm |
So for this one, I am kicking myself, because we updated like a zillion core views so of course it would also need updates in contrib |
| xjm |
I think Views itself should inform people about this |
| xjm |
The CR should at least mention it |
| xjm |
There does appear to be an upgrade test already |
| Björn Brala (bbrala) |
It should have been a deprecation warning in the module if that setting is used i guess, then remove in next major. Or wont that work? |
| Björn Brala (bbrala) |
"wouldn't that have worked"* |
| xjm |
This seems to be exactly related @Gábor Hojtsy (he/him)’s point about default config actually |
| Björn Brala (bbrala) |
🙂 |
| xjm |
The update hook should have fixed it on runtime, but wouldn't have addressed default config or test fixtures. We're expecting contrib to update based on the fact that tests should fail, but what if they don't have tests, or if it's optional config? |
| Björn Brala (bbrala) |
Then it will just fail when it is used, which is kinda frustrating. |
| xjm |
/cc @longwave whom we also discussed this issue with |
| Björn Brala (bbrala) |
1.6k - https://git.drupalcode.org/search?group_id=2&scope=blobs&search=-path%3A...…]eb+-path%3Aprofiles+-path%3Asites+default_argument_skip_url |
| xjm |
The CR also appears to be incorrect; it says the change was in 10.2.0 but it was never backported |
| xjm |
So maybe the solution is to put something in 10.3 for it too |
| xjm |
At the least, the CR should be updated to tell people to fix their default and test fixture config |
| Björn Brala (bbrala) |
In my humble opinion this is quite the work we just threw over the fence to contrib. |
| xjm |
Yeah, we didn't think about it the right way. We were thinking about only removing a broken API. This issue is not like a normal config change, because it was in 11.x only and we removed it outright without any replacement due to the brokenness. |
| xjm |
We didn't think about the fact that it gets written to config automatically, despite the MR having dozens of such changes. :stuck_out_tongue: |
| xjm |
Err the patch |
| Björn Brala (bbrala) |
hehe, things happen :wink: |
| Björn Brala (bbrala) |
Helping contrib to fix this though update-bot is possible. We could detect it, (name is pretty predictable), we could even fix is (jq is very helpfull). But its work that needs doing quickly then |
| Björn Brala (bbrala) |
Also, will it default to a value if it is not in the yml? (edited) |
| Björn Brala (bbrala) |
then it wont mean no more compatibility with older core versions. (edited) |
| longwave |
doesn't views_view_presave handle the default/optional/etc config cases? iirc it invokes ViewsConfigUpdater which we changed here and handles things like this |
| longwave |
so if that key exists on disk then it's discarded whenever the view is saved to active config |
| xjm |
Right, but that doesn't help with the actual saved fixtures in the contrib code |
| xjm |
It's being conflated with "deprecation" since we didn't backport anything |
| xjm |
But it still is kinda-sorta... somethign |
| longwave |
we could improve the change record but iirc for other Views config changes this is all we do |
| Björn Brala (bbrala) |
there have been more? |
| Björn Brala (bbrala) |
do those have cr's? Or did they just change? |
| Björn Brala (bbrala) |
https://www.drupal.org/list-changes/drupal/published?keywords_descriptio...…]3E%3D&created%5Bvalue%5D=&created%5Bmin%5D=&created%5Bmax%5D= |
| xjm |
I would look at Views update hooks |
| Björn Brala (bbrala) |
Dont see any searching for views in the cr's |
| Björn Brala (bbrala) |
ah good one |
| longwave |
#2831233: Field tokens for "historical data" fields (revisions) contain a hyphen, breaking twig templates and throwing an assertion error didn't have a CR but modified Views config |
| longwave |
not at computer to easily check but ViewsConfigUpdater has a bunch of these sorts of things, BC layers for changes we made in Views |
| xjm |
Maybe that's what we should have done for the URL thingy |
| longwave |
https://www.drupal.org/node/3212351 added a Views setting (amongst other things) and modifies configs and the change record mentions nothing about Views |
| xjm |
Check out ViewsConfigUpdater::needsEntityLinkUrlUpdate(). In that case, we had the fix-it method, and the fix-it method was what was deprecated. |
| xjm |
And the second example adds a MediaConfigUpdater along similar lines |
| longwave |
re the original issue I remember discussing this with Lendude in Vienna and we decided no need to deprecate as this is completely broken and has never worked since Views in D7 |
| xjm |
Yeah I agreed and still agree with that decision... I think |
| xjm |
But apparently contrib is finding it at least moderately disruptive |
| longwave |
I am not sure what Berdir means there or what problem it is actually causing |
| Björn Brala (bbrala) |
The gist is, should changes like those go through the normal deprecation cycle. Where modules might catch this before removing it completely. That is basically what seems to have been skipped here. |
| longwave |
(in the linked Slack thread) |
| xjm |
Again though, like I said in the other thread, we haven't done that for data. Which is essentially what @longwave is saying too. |
| xjm |
However, the approach of adding a helper method to clean up your default views on install so you don't have to change them, and then removing that method in a later major, seems like something we could do here. (edited) |
| xjm |
We just didn't do it here because the thing was wholly dead-code broken |
| longwave |
that's what ViewsConfigUpdater does already, except I'm not sure if/when we remove old changes from it |
| longwave |
how long do we support config that may exist on disk and may be imported at any future time especially if contrib doesn't always have tests to discover this sort of thing |
| xjm |
Ohh I missed it, there is an addition in ViewsConfigUpdater. Sorry. Maybe we should go back and ask @berdir what specifically was disrupted. |
| xjm |
Since it runs always and not just in an update hook, it seems like that should have mitigated the disruption until such time as we deprecate that support. |
| xjm |
I added a comment in the original thread |
| xjm |
...which was in this channel anyway. Oh well. |
| xjm |
So TLDR, sounds like this would simply be helpful to add manual detection for, rather than being a missing deprecation API or something. Because it is a one-off removal of broken code with a major-only schema change, rather than a deprecation. (edited) |
| catch |
#3086261: [policy] Add proper deprecation notices in config entity presave bc layers |
| xjm |
Brilliant, that is exactly it. And also, lol. |
| xjm |
Going to stick that on the D12 should-haves at minimum. |
| catch |
Oof how many duplicate issues have I opened for the same problem #2913850: Trigger E_USER_DEPRECATED from config entity presave bc layers |
| xjm |
:joy: |
| catch |
At least three. #3443942: Add proper deprecation notices in config entity presave bc layers |
| xjm |
@catch I will let you pick which one to keep :rolling_on_the_floor_laughing: |
| xjm |
Maybe one that hasn't been updated across ten different branches is less depressing |
| catch |
#3442395: ckeditor5 and editor module test config exports/stubs rely on hook_editor_presave() bc layers is also related.So we could retrospectively add some of these to 10.3 and 10.4 where they've been missed, which is quite a lot of places.And yeah maybe keeping the newest one in this case is less painful. |
| catch |
Left #3443942: Add proper deprecation notices in config entity presave bc layers as the open issue. |
| catch |
The newer problem is that now that we're adding config validation, if you have old config with crufty stuff in it that's not recognised, that will start failing tests etc. (I didn't check if that's the problem in the issue this thread was originally discussing or not). So we actually need to do this properly from now on, and ideally retrospectively in 10.x too. |
| Gábor Hojtsy (he/him) |
Proposed by @Björn Brala (bbrala)i've seen 2 instances recently where changes to .yml give issues. One by berdir with removal of default_url in views, one with definition of new api for categories: https://www.drupal.org/node/3375748How can we support those and make those detectable. |
| Gábor Hojtsy (he/him) |
Are we specifically talking about plugin YAMLs or YAMLs in general (eg. config too)? (edited) |
| Gábor Hojtsy (he/him) |
I think plugin YAMLs are relatively easy because they would have a simpler structure, so the methods used in potx to address them could be reused? |
| Gábor Hojtsy (he/him) |
See https://git.drupalcode.org/project/potx/-/blob/7.x-3.x/yaml_translation_... |
| Gábor Hojtsy (he/him) |
(not exactly the same format of course, but a similar simple solution may work) |
| mikelutz (he/him) |
Plugin yamls are easy because the yaml is passed to a class as config, which can detect deprecated keys in the constructor and issue a deprecation notice. |
| mikelutz (he/him) |
The harder one is configuration yamls. We change the structure, run an update hook to manage the change on live sites, but a module that bundles config has no idea that their config is now invalid. Finishing config validation will help here, as we could have tools to run bundled config in modules against the validation, and they would get an error against new versions of core where the validation rules have changed. |
| Gábor Hojtsy (he/him) |
@mikelutz (he/him) re plugins, I think "the bot" will not instantiate all the plugins probably, although upgrade status can try to instantiate all the plugins somehow and catch deprecation errors, hm |
| Gábor Hojtsy (he/him) |
it will be simpler/faster if we would have a quick static way to detect the deprecated keys |
| mikelutz (he/him) |
Well, that's true, I'm assuming you have testing in your module to cover your yaml plugins. If you are limiting yourself to static analysis without unit tests, then that does get trickier. |
| Björn Brala (bbrala) |
In the linked issue a config key was removed. And if a module had config it broke.Seems like if we are to remove config Keys that break if in config it would be nive to be able to anlyze that. |
| Björn Brala (bbrala) |
Ow wow, sorry my comment was written before your conversation :sweat_smile: |
| Gábor Hojtsy (he/him) |
@Björn Brala (bbrala) we may be able to use parts of the config schema API? (edited) |
| Björn Brala (bbrala) |
Yeah tests should preferably be used. It's about bundled config, so I guess config validation would at some point make validation possible. |
| Björn Brala (bbrala) |
We could probably? But still needs quite some work.#3427641: [meta] Config validation for a more reliable Drupal + reliable Recipes from the start |
| Gábor Hojtsy (he/him) |
No I mean specifically use the config schema system to define the deprecated keys in either core or in upgrade status |
| Gábor Hojtsy (he/him) |
If we need it done ASAP, that is in the Drupal 10 to 11 upgrade path, we need it in upgrade status probably, not in core. |
| Björn Brala (bbrala) |
I think the impact is relatively low, but i can imagine it being a nuisance for module developers. For that specifc annotation it would've been easier when its an attribute i think, since those should just go through phpstan. It wouldn't have been an issue then im thinking.So perhaps we can just ignore this for now, and next cycle we'll have attributes for at least this part. |
| Björn Brala (bbrala) |
I'll keep an extra eye out on the change records to see if any other like this pop up. |
| xjm |
OK weird, I had thought we'd already introduced deprecatibility for all these APIs (also I just coined the word "deprecatibility") |
| xjm |
Oh except config -- which, yes, validation is the answer there IMO |
| Björn Brala (bbrala) |
In the linked change record is is changing the value type of a field in an annotation. |
| Björn Brala (bbrala) |
This is the thread that spawned this thread: https://drupal.slack.com/archives/C03L6441E1W/p1716293607661759 |
| Gábor Hojtsy (he/him) |
@xjm problem with relying on validation is we check for deprecated APIs in the current major for the future major, and the current major should consider the config valid... does config validation have a deprecation category / can be made to throw deprecation exceptions? |
| xjm |
@Gábor Hojtsy (he/him) Well config being data means it should have upgrade paths when we tighten validation, but yes I see what you mean |
| xjm |
For D11, I think we might need to handle all this in the PHP layer, since we are already in beta |
| xjm |
The deprecation APIs we introduced for yaml for things like services and libraries and so on were beta-blocker requirements for the respective major versions (edited) |
| Björn Brala (bbrala) |
so basically, no way out right now, but something to consider for 11. :x |
| xjm |
Yep, I would add it to the D12 roadmap |
| Gábor Hojtsy (he/him) |
hm, how does default shipped config get resolved by an upgrade path? |
| xjm |
We are talking about different things -- I think each needs its own thread |
| xjm |
Plugins and annotation/attribute values are one thing, config schemata another, config data itself yet another, etc. |
| xjm |
So maybe the two problems identified so far should each get their own thread for now, and we add more if we find other regressions |
| xjm |
Also I think the two problems above should get core issues if they don't already have them, be tagged contrib blockers, and be added to the D11 beta2 meta |
| Björn Brala (bbrala) |
i'll try and make/search for the core issues this week. |
| Björn Brala (bbrala) |
and tag them |
| xjm |
@Gábor Hojtsy (he/him) WRT default config, theoretically it gets installed when the module is installed, so we could ahve a PHP/runtime deprecation warning for that as a workaround |
| xjm |
Since modules are installed each time in each test, I mean |
| xjm |
It's not as nice as having static analysis directly on the config, of course, but maybe that combined with upgrade status support for weirder/bad cases might help? |
| Gábor Hojtsy (he/him) |
not all default config is installed 🙂 |
| xjm |
Well it's all installed under some circumstances |
| xjm |
Like optional config is based on another module being installed... are there any cases when it's not? Hmm I haven't thought about that since like... Drupal 8... |
| xjm |
Actually I have no idea how contrib tests that |
| xjm |
OK yeah, tricky |
| xjm |
If it's a core module it's easy to test -- if the contrib module has tests -- but if it's another contrib module that's even less reliable |
| xjm |
Do we actually have any cases in D11 where default config is a problem though? Sorry I lost track of what the second issue was |
| xjm |
https://www.drupal.org/node/2442603 was linked but that is pre-8.0.0 so I don't understand |
| Björn Brala (bbrala) |
Ignore the second issue in that link. |
| Björn Brala (bbrala) |
Was mostly triggered by the first. Which is annotation/attribute |
| Björn Brala (bbrala) |
The other issue was in views where default_url was removed. |
| Björn Brala (bbrala) |
Let me try and find it. |
| Björn Brala (bbrala) |
https://www.drupal.org/node/3382316 |
| Björn Brala (bbrala) |
So that was mentioned by eh, i think @berdir? That after campatibility fixes that came up. |
| Björn Brala (bbrala) |
https://drupal.slack.com/archives/C03L6441E1W/p1716200019951969 |
| xjm |
Ohh I vaguely remember talking about that at DDD |
| xjm |
Well I am glad I asked them to at least make a CR. :joy: |
| Björn Brala (bbrala) |
haha :smile: |
| xjm |
@Gábor Hojtsy (he/him) Can we have one thread for the plugin issue and another for the views issue? |
| xjm |
I want to go into details on both |
| xjm |
@Gábor Hojtsy (he/him) NM Björn has us covered |
| xjm |
Sorry didn't notice how late it's gotten |
| Gábor Hojtsy (he/him) |
proposed by @mikelutz (he/him) #3108658: Handling update path divergence between 11.x and 10.x |
| mikelutz (he/him) |
Need some guidance on where to put effort here, how to handle drush, whether we could convert the service call in the old major to attributes, or if we should move the responsibility of turning the later hook into a noop into the hook itself rather than the updater, since drush has its own, and there could be other custom implementations. |
| xjm |
I would STRONGLY encourage us to go with "minimum possible change" for the issue and descope rearchitecting it better-er to a followup |
| xjm |
Also wow this issue has like 5000 comments I haven't read just in the past week |
| xjm |
@catch’s feedback on the new approach would be a big help here |
| mikelutz (he/him) |
There’s more discussion here: https://drupal.slack.com/archives/C1BMUQ9U6/p1716720512074899 |
| mikelutz (he/him) |
But going over Alex’ approach, I realized it means we have to rewrite the runner in drush too, and set up composer requirements such that your drush version matches the core version of the updater. |
| xjm |
I can't speak to Drush, but yes I'd assume it'd need a followup. Which is another reason to get this in before beta2 in the next few weeks. |
| mikelutz (he/him) |
Which leads me to the next part of this: minimum possible change would be to do nothing to core, and keep it all inside the update hooks. With no change to core at all, you would backport 11001 to 10301 and add \Drupal::keyValue->get('updates_to_skip')->set(11001, TRUE); to the 10301 and wrap all the code in 11001 with if (!\Drupal::keyValue->get('updates_to_skip')->get(11001, FALSE) { … } |
| mikelutz (he/him) |
It’s a pain DX, but would guarantee the future version wouldn’t do anything regardless of the runner used. |
| xjm |
That is... a very good approach actually. Onus on committers to not let anyone eff it up, but that's true in general. :joy: |
| xjm |
I like that because it's easy to understand and we already have similar patterns in core where we've done conditional updates based on the state of the site |
| mikelutz (he/him) |
right, and you wrap the 11 code if there’s any chance of backporting at all. if you never backport, then no big deal. |
| xjm |
The existing API is yucky and sometimes brittle but it does have the advantage of being the devil we know in terms of edgecases |
| mikelutz (he/him) |
And even then, if you can make the hook idempotent, you wouldn’t even need that. if the hook is something like “If A exists, move A into B” then it’s fine to just let it run again, as the next time, A won’t exist so it already won’t do anything. |
| xjm |
if statements wrapping update hooks' function body is already something we should do more of; it makes them more resilient (edited) |
| mikelutz (he/him) |
Right, :joy: I got a chance to explore it while trying my approach. :joy::joy:. I now want to burn it down and rebuild it sometime in the 11.x cycle. But not here. |
| catch |
The trouble with keeping the logic in the update hooks is it creates a chicken and egg between the 10-11 updates. Unless we synchronise commits all the time |
| xjm |
Well in @mikelutz (he/him)’s example the D11 hook only needs to know its own number |
| xjm |
Only the D10 version needs to prognosticate |
| mikelutz (he/him) |
Well, the other issue is it doesn’t deal with the whole issue of the hook being in 11.1, but not 11.0 |
| mikelutz (he/him) |
But I’m still of the opinion we should find a way require 10.4 to go to 11.1 and 10.5 to go to 11.2 as a whole, though it’s tricker than it sounds. |
| mikelutz (he/him) |
Then again, the combination of the two might be easiest. Go with Alex’ service addition, and the check for missing updates in system.install, but just check update_hook_registry->getUpdateSkipped() in the future update hook instead of messing with update_do_one. Then we don’t have to deal with drush, and the future update hook won’t do anything regardless of how it gets executed. IT’s a minimal change to core, just to add some helpers, but doesn’t change anything with the update api at all, still keeps all the runtime responsibility in the hooks themselves. |
| mikelutz (he/him) |
Add the check in d11 if there’s any chance of backporting and even then only if the hook itself is not idempotent and would break something if it ran twice. |
| Björn Brala (bbrala) |
https://www.drupal.org/node/3382316 |
| Björn Brala (bbrala) |
Split from 3️⃣ |
| Björn Brala (bbrala) |
Original source: https://drupal.slack.com/archives/C03L6441E1W/p1716200019951969 (edited) |
| xjm |
So for this one, I am kicking myself, because we updated like a zillion core views so of course it would also need updates in contrib |
| xjm |
I think Views itself should inform people about this |
| xjm |
The CR should at least mention it |
| xjm |
There does appear to be an upgrade test already |
| Björn Brala (bbrala) |
It should have been a deprecation warning in the module if that setting is used i guess, then remove in next major. Or wont that work? |
| Björn Brala (bbrala) |
"wouldn't that have worked"* |
| xjm |
This seems to be exactly related @Gábor Hojtsy (he/him)’s point about default config actually |
| Björn Brala (bbrala) |
🙂 |
| xjm |
The update hook should have fixed it on runtime, but wouldn't have addressed default config or test fixtures. We're expecting contrib to update based on the fact that tests should fail, but what if they don't have tests, or if it's optional config? |
| Björn Brala (bbrala) |
Then it will just fail when it is used, which is kinda frustrating. |
| xjm |
/cc @longwave whom we also discussed this issue with |
| Björn Brala (bbrala) |
1.6k - https://git.drupalcode.org/search?group_id=2&scope=blobs&search=-path%3A...…]eb+-path%3Aprofiles+-path%3Asites+default_argument_skip_url |
| xjm |
The CR also appears to be incorrect; it says the change was in 10.2.0 but it was never backported |
| xjm |
So maybe the solution is to put something in 10.3 for it too |
| xjm |
At the least, the CR should be updated to tell people to fix their default and test fixture config |
| Björn Brala (bbrala) |
In my humble opinion this is quite the work we just threw over the fence to contrib. |
| xjm |
Yeah, we didn't think about it the right way. We were thinking about only removing a broken API. This issue is not like a normal config change, because it was in 11.x only and we removed it outright without any replacement due to the brokenness. |
| xjm |
We didn't think about the fact that it gets written to config automatically, despite the MR having dozens of such changes. :stuck_out_tongue: |
| xjm |
Err the patch |
| Björn Brala (bbrala) |
hehe, things happen :wink: |
| Björn Brala (bbrala) |
Helping contrib to fix this though update-bot is possible. We could detect it, (name is pretty predictable), we could even fix is (jq is very helpfull). But its work that needs doing quickly then |
| Björn Brala (bbrala) |
Also, will it default to a value if it is not in the yml? (edited) |
| Björn Brala (bbrala) |
then it wont mean no more compatibility with older core versions. (edited) |
| longwave |
doesn't views_view_presave handle the default/optional/etc config cases? iirc it invokes ViewsConfigUpdater which we changed here and handles things like this |
| longwave |
so if that key exists on disk then it's discarded whenever the view is saved to active config |
| xjm |
Right, but that doesn't help with the actual saved fixtures in the contrib code |
| xjm |
It's being conflated with "deprecation" since we didn't backport anything |
| xjm |
But it still is kinda-sorta... somethign |
| longwave |
we could improve the change record but iirc for other Views config changes this is all we do |
| Björn Brala (bbrala) |
there have been more? |
| Björn Brala (bbrala) |
do those have cr's? Or did they just change? |
| Björn Brala (bbrala) |
https://www.drupal.org/list-changes/drupal/published?keywords_descriptio...…]3E%3D&created%5Bvalue%5D=&created%5Bmin%5D=&created%5Bmax%5D= |
| xjm |
I would look at Views update hooks |
| Björn Brala (bbrala) |
Dont see any searching for views in the cr's |
| Björn Brala (bbrala) |
ah good one |
| longwave |
#2831233: Field tokens for "historical data" fields (revisions) contain a hyphen, breaking twig templates and throwing an assertion error didn't have a CR but modified Views config |
| longwave |
not at computer to easily check but ViewsConfigUpdater has a bunch of these sorts of things, BC layers for changes we made in Views |
| xjm |
Maybe that's what we should have done for the URL thingy |
| longwave |
https://www.drupal.org/node/3212351 added a Views setting (amongst other things) and modifies configs and the change record mentions nothing about Views |
| xjm |
Check out ViewsConfigUpdater::needsEntityLinkUrlUpdate(). In that case, we had the fix-it method, and the fix-it method was what was deprecated. |
| xjm |
And the second example adds a MediaConfigUpdater along similar lines |
| longwave |
re the original issue I remember discussing this with Lendude in Vienna and we decided no need to deprecate as this is completely broken and has never worked since Views in D7 |
| xjm |
Yeah I agreed and still agree with that decision... I think |
| xjm |
But apparently contrib is finding it at least moderately disruptive |
| longwave |
I am not sure what Berdir means there or what problem it is actually causing |
| Björn Brala (bbrala) |
The gist is, should changes like those go through the normal deprecation cycle. Where modules might catch this before removing it completely. That is basically what seems to have been skipped here. |
| longwave |
(in the linked Slack thread) |
| xjm |
Again though, like I said in the other thread, we haven't done that for data. Which is essentially what @longwave is saying too. |
| xjm |
However, the approach of adding a helper method to clean up your default views on install so you don't have to change them, and then removing that method in a later major, seems like something we could do here. (edited) |
| xjm |
We just didn't do it here because the thing was wholly dead-code broken |
| longwave |
that's what ViewsConfigUpdater does already, except I'm not sure if/when we remove old changes from it |
| longwave |
how long do we support config that may exist on disk and may be imported at any future time especially if contrib doesn't always have tests to discover this sort of thing |
| xjm |
Ohh I missed it, there is an addition in ViewsConfigUpdater. Sorry. Maybe we should go back and ask @berdir what specifically was disrupted. |
| xjm |
Since it runs always and not just in an update hook, it seems like that should have mitigated the disruption until such time as we deprecate that support. |
| xjm |
I added a comment in the original thread |
| xjm |
...which was in this channel anyway. Oh well. |
| xjm |
So TLDR, sounds like this would simply be helpful to add manual detection for, rather than being a missing deprecation API or something. Because it is a one-off removal of broken code with a major-only schema change, rather than a deprecation. (edited) |
| catch |
#3086261: [policy] Add proper deprecation notices in config entity presave bc layers |
| xjm |
Brilliant, that is exactly it. And also, lol. |
| xjm |
Going to stick that on the D12 should-haves at minimum. |
| catch |
Oof how many duplicate issues have I opened for the same problem #2913850: Trigger E_USER_DEPRECATED from config entity presave bc layers |
| xjm |
:joy: |
| catch |
At least three. #3443942: Add proper deprecation notices in config entity presave bc layers |
| xjm |
@catch I will let you pick which one to keep :rolling_on_the_floor_laughing: |
| xjm |
Maybe one that hasn't been updated across ten different branches is less depressing |
| catch |
#3442395: ckeditor5 and editor module test config exports/stubs rely on hook_editor_presave() bc layers is also related.So we could retrospectively add some of these to 10.3 and 10.4 where they've been missed, which is quite a lot of places.And yeah maybe keeping the newest one in this case is less painful. |
| catch |
Left #3443942: Add proper deprecation notices in config entity presave bc layers as the open issue. |
| catch |
The newer problem is that now that we're adding config validation, if you have old config with crufty stuff in it that's not recognised, that will start failing tests etc. (I didn't check if that's the problem in the issue this thread was originally discussing or not). So we actually need to do this properly from now on, and ideally retrospectively in 10.x too. |
mikelutz, xjm, catch, Kristen Pol, bbrala, Gábor Hojtsy, andypost, Berdir, longwave
The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.
Comments
Comment #2
bbralaSubject:
YAML deprecations: Should we have a process around those, i've seen 2 instances recently where changes to .yml give issues. One by berdir with removal of default_url in views, one with definition of new api for categories: https://www.drupal.org/node/3375748
How can we support those and make those detectable.
Comment #3
gábor hojtsyComment #10
mikelutzComment #11
smustgrave commentedWent into #d11readiness
Verified threads were captured and active participants credited.
Comment #13
xjmAmending attribution.