Hi! Thanks for your request and interest in this module. Really appreciate all the contributions you’ve made.
I won’t undercut Jaypan and directly make you a co-maintainer. But I will say I’ve finally got some time for this module again, and will be working the issue queue next week for a client gig. So I can at least move a bunch of your MRs forward (and hopefully cut a new release) in a few days.
Thanks again!
-Derek
D7 is EOL. (Finally) doing some cleanup on the update.module component... 😅
This seems duplicate with 🐛 Code Review for Update module Closed: outdated . Regardless, both are "outdated" now, and none of these suggestions are still relevant.
Most of this code no longer exists. Indeed, we're not using t() in tests. All of the code style stuff has long been fixed as this was ported to D8 and beyond.
Indeed, this is no longer relevant. The "Update Manager" UI provided by update.module is now gone from core.
Indeed, this was actually a problem with the XML release history feeds provided by update.drupal.org, which was fixed on January 22nd of this year. Closing this as outdated.
I know it's confusing, but update.php
and the database update UI is a separate component from update.module, which is only the "Update status" module that provides the "Available updates" report at /admin/reports/updates (and in earlier versions of core, the "Update Manager" UI that tried to install available contrib updates on your webserver).
phpunit (next minor) is failing, but that seems to be about new 11.2.x deprecations, so I don't think that's anything I need to worry about here. Everything else is now green.
Wasn't really sure what to call it, but here's a start. Working fine in production on the site where I needed this.
dww → created an issue.
From #3007167-30: [META] Enable layout builder for form displays, and deprecate field_layout → , @bnjmnm already created https://www.drupal.org/project/field_layout → -- hopefully they're still willing to maintain that.
Is this duplicate with 📌 Inform users that Field Layout is deprecated Active ?
I believe it's blocked on 📌 Automated Drupal 11 compatibility fixes for duration_field Needs review to actually be useful.
Re: #143/#144: Or don't start chromedriver locally and try to run any FunctionalJavascript test.
Thanks for working on this. However, since it's entirely dead code at this point, and there's no explanation for what the actual problem is, I'm going to mark this "won't fix". I hate to see anyone spending more time on a dying / dead system.
Re: #5: hah, fair enough. 😅 Apologies for my less-than-tactful wording, and thanks for not taking it personally!
Thanks for doing a trial run like this! Very helpful and encouraging that the parent issue is on the right track.
If a branch off an issue branch confuses the tooling so much, probably not the end of the world to have an initial commit of ✨ Add drupalGet() to KernelTestBase Active in here, no? The changes tab will be bloated, but it should be fairly simple to review changes via each proceeding commit...
Anyway, thanks again!
Bot is happy and green. I guess this is a new feature. 😅
I'm probably forgetting something about how this could break things for existing sites with existing views config. Especially while 🐛 Views handler loading should respect configuration Active is unresolved. But it's here for folks that want to try it out. Maybe I'm over-thinking it, and this is harmless for legacy sites.
FYI: I opened 📌 Use the new 'entity_reference' Views filter in EntityViewsData::processViewsDataForEntityReference() Active which should help considerably. 😅
dww → created an issue.
dww → created an issue.
Probably duplicate.
Thanks! Pipeline is green. I’ve read the diff many times now. Nothing left to improve to my eyes, other than the follow-ups. 😉 let’s get this in.
Thanks for your patience and persistence!
cspell failed on the latest. Added another MR suggestion to fix it. So very close! 😅
Just marked
🐛
Users can edit/delete content but not unpublish it
Active
duplicate with this, which had 35 followers.
I imagine there are other duplicates lurking in the issue queue for this.
This is a very common need. Further evidence core should provide this out of the box.
Thanks!
-Derek
IMHO, yes, this is something core should do. Reading the comments, we've got "yes this is a good idea" from all sorts of folks who are either past or current release managers, product managers, etc. It's fairly absurd that we have granular permissions for delete, but not publish/unpublish. Yes, content moderation provides more advanced publishing workflows. But for something so fundamental to a content management system as publish status, I think core needs to solve this.
Thanks for opening those followups. Resolved my open threads. I'll let @nicxvan apply the phpcs suggestion, then it's RTBC to me.
Thanks!
-Derek
Thanks for opening this. I'm not even really sure *where* to document this. 😅 I guess that's one of the remaining tasks...
Thanks for opening this issue based on my feedback at #3489141.
Sad panda. I wish we were consistent with this sort of thing. It's too bad we released Hook
with this slightly weird DX. It's definitely minor, no real harm done the way it is, other than the mild WTF.
If this is "[PP-2]", it should be postponed, and the summary should say what it's blocked on.
Thanks,
-Derek
Looks like we need a few follow-ups here:
- Something to document how to put form submit callbacks into
Hook
classes - Fixing
Hook
to use NULL for both of its optional parameters, instead of''
for$method
vsNULL
for$module
Otherwise, I think this is RTBC.
Humans are smarter than bots. 😂 I still have questions and concerns. But we're really close now.
Thanks!
-Derek
Late to the game, but this isn't a bug. Nothing was broken. It's just removing dead code. Hence, task.
Removing duplicate related issue
Nice, didn’t know we had that. 😅 TIL. Thanks!
I believe you meant to post that at 📌 [PP-1] Determine how to implement Form Alters with attributes. Active , right?
Great work here! The change is exactly the right fix for random fails in JS tests: if we're trying to use an element, we need to wait for the AJAX request to complete before we proceed. As @catch's initial attempt shows, we can't just wait on the field itself -- it's already there. 😅 We need to wait on the AJAX request that clicking that field triggers.
The 2 repeat MRs show that the fix is indeed solving the problem.
Nothing left to improve. RTBC.
Thanks!
-Derek
Thanks! Overall, this looks really good and clean. But (as usual) I've got some concerns and nits on the MR. 😅
Initial pass at saving credits.
Yes, indeed. And there are some open questions at 📌 Rename update module back to Update Status Active that I would appreciate some input on.
Thanks!
-Derek
Thanks for adding the CR and committing this. Hopefully this helps more than it confuses. 🤞
Yay, thanks! I published the CR
Hi folks, thanks for working on this. Apologies this issue fell through the cracks. There's a ton of existing work towards implementing this here:
Let's continue over there if anyone's interested in moving this along.
Thanks!
-Derek
Huzzah, thanks!
Re:
except probably exceptions need constructor to throw deprecations
From @catch via Slack:
https://drupal.slack.com/archives/C079NQPQUEN/p1741811869453559?thread_t...
catch: I don't think we have anything explicit for deprecating exceptions, but given these aren't used anywhere outside the system we're deprecating I think just @trigger_error() is fine.
dww: But then I have to add __construct()
methods. 😅 Can I just do the annotation, instead?
catch: Sorry that's what I meant...
📌 Deprecate authorize.php and the FileTransfer system Active is now ready for review.
Closed the MR here since it was for the 'Remove all the things' approach, before we decided to split this up into a plan with multiple phases.
Thanks,
-Derek
Added another API change point for:
- Every method in
core/modules/update/update.manager.inc
Everything is now done. Pipeline is green. Simplified remaining tasks.
This is now ready for review (both the MR and the CR). Unassigning.
Thanks!
-Derek
Added an item to API changes for:
- Every method in
core/modules/update/update.authorize.inc
Updating remaining tasks to cross off what's done. Almost there...
@andypost: Argh, what are you doing? I assigned this to myself and am actively working on it.
Okay, cool, that's working now.
Adding a TODO list under remaining tasks, and crossing out what's already been pushed...
@andypost, see:
https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →
I’m assigning myself, since I’ve got more of this done locally. Stay tuned.
I asked in #core-development in Slack, but x-posting here:
core/modules/system/tests/src/Functional/System/SystemAuthorizeTest.php
is now failing since it's triggering deprecations. Can I completely remove that test as part of this issue, or do I need to convert it to expect deprecations, etc?
Opened a CR for this (to get a valid NID for deprecation messages) and started deprecating everything in the IS. Pushed commits for authorize.php
and system_authorized_*()
. Opened an MR. Still incomplete, so NW, but wanted to at least get this started and to see if the bot explodes in any unexpected ways. 😅
Tagging as a 11.2.0 release priority. See 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and friends.
smustgrave → credited dww → .
Rebased. Pipeline is green.
@andypost: Not sure what you're requesting in #3. That MR is for a set of changes to completely remove all this code. Instead, it'll probably be more simple to start over from scratch here, since there's now a different scope and approach needed for this issue.
But if I'm misunderstanding, please let me know.
Thanks!
-Derek
Blocker is in. Just need trivial rebase and those should be green pipelines
Great, thanks!
Dare we mention any of the motivations in either the CR or release note? That composer is the only supported way to safely manage dependencies? And to pave the way for “automatic updates”? Although the wording / branding is going to be confusing with that, since the new thing is now being called “Update Manager” 😂
Also, I’m not sure we should say “drush or manual updates” in the CR. I thought we want to discourage “manual updates”, and I don’t think drush has a solution for this, either.
Groovy, thanks!
UpdateManagerUpdateTest
is failing now, since it's assuming the old default.
However, that test is completely removed @ 📌 Remove UI and routes for the ability to update modules and themes via update.module and authorize.php Active .
Should we bother fixing the test here and needing to re-roll one of these MRs whenever the other lands, or should we postpone this on committing #3502973 first?
LGTM, but I haven't directly contributed to Drupal CMS config / recipes yet, so I don't feel super qualified to RTBC this.
Only question is if this will still work if the default in core changes, too: 📌 Consider flipping the default for update.settings:check.disabled_extensions Active
The UI for when you turn this on isn't terrible. 😅 The uninstalled stuff appears in a whole separate table at the bottom of the report. So hopefully won't be too much confusing noise in the regular case.
In general, +1 to the change.
I won't merge this until 📌 Enable GitLab CI and cleanup build pipelines Active is done and we have passing green pipelines on all relevant versions.
Yay, thanks!
Thanks for working on this! I should have mentioned all these changes could have been selectively merged into this fork from all the commits I pushed to the parent issue. Hope you didn't completely duplicate the effort. 😅
But thrilled to see progress here without me. 😉 I've been way too slammed on way too many fronts to work on this.
Thanks again!
-Derek
TODO: See comment #4, points 1 and 4 (at least).
Thanks for working to smash this bug!
Latest pipeline failed in phpstan. We need to see green testbot results before this is ready for review. In this case, hopefully a failing test-only run, too.
Thanks,
-Derek
Quick driveby review. A few pedantic nits for now. Haven’t finished, but this is all I could do from my phone while eating lunch with the other hand. 😂
I'm not so sure. 😅 For 📌 Remove adding an extension via a URL Fixed we removed the routes entirely, without deprecation. True, we didn't have a mechanism at the time, but from a release manager and framework manager standpoint, this stuff is so old, problematic, and presumably unused, that it's better to hide it from end users entirely than to keep the functionality visible (even if there are warnings).
Coming here from https://drupal.slack.com/archives/CGKLP028K/p1739920511290749
I don't see this mentioned anywhere at https://project.pages.drupalcode.org/gitlab_templates/jobs/upgrade-status/ or otherwise. Google search is failing me. I've got a project I'm getting pinged about for a D11 issue. I'd love to run the upgrade status job via the bot. But if I do that, it's defaulting to the current core (11.1.x) and trying to check for D12 compatibility. I started a manual pipeline, and I didn't see any way to control "No, I want you to use D10 to run upgrade status to confirm D11 is cool".
- How do you do this?
- Shouldn't this be the default config for Upgrade status jobs? :sweat_smile:
The answer is when creating a manual pipeline, although there's no obvious choice in the initial list of variables to configure, if you manually set DRUPAL_CORE
at last row of the form to ^10
or 10.4.2
or whatever you want, and then tweak the visible RUN_JOB_UPGRADE_STATUS
variable to 1, you'll get what you need (a test run against previous major that then checks upgrade_status against the current major you're porting to)...
Saving credit
Done, thanks!
Tagged https://www.drupal.org/project/condition_query/releases/8.x-1.5 → so I don't forget. 😂
Pushed some more commits. Test seems to be working now on all versions.
Nope, still failing. I'm way out of time for this. If anyone wants to get the Kernel test passing on D11, knock yourself out. 😉
Thanks/sorry,
-Derek
Let's see if that solves it... 🤞
Probably https://www.drupal.org/node/3337193 → is the culprit
Not sure why, and don't have time to further investigate, but the tests are still failing on D11, even with compatibility declared.
Fixed cspell and phpcs.
phpunit is failing on D11, since
📌
Automated Drupal 11 compatibility fixes for condition_query
Needs review
isn't merged.
Everything else seems okay, so I merged this to 8.x-1.x.
@mvonfrie, re #45: Feel free to open another postponed child issue about that, much like ✨ Add $entity->toUrl() and $entity->toLink() methods to allowed methods list in Twig sandbox policy Needs work . Once this issue is merged and fixed, it'll be much easier and safer to expand the allowed methods and target specific interfaces where we want to allow other methods.
Thanks,
-Derek
Thanks for working on this! Working on an MR review. About to open a bunch of threads. But moving out of RTBC queue for now so a core committer doesn't spend time looking just yet...
This is still causing troubles in contriblandia. See 🐛 Fix Deprecation warning for profiles view for Drupal 10.3 Active . We're trying to fix a deprecation that requires changing default views. Due to 🐛 Views handler loading should respect configuration Active , it's "safe" to update default views provided by contrib modules to use the new handler IDs, since the actual handler used is determined by the Views Data API, not the views config (until 11.2.0 when #3458099 will be released). However, if you change the config, and run tests against 10.2.x core, the plugins don't exist, and the whole test run explodes due to strict config schema errors. See https://git.drupalcode.org/issue/profile-3465501/-/jobs/2317666
I'd love it if there was an easy way to globally set $strictConfigSchema = FALSE
for an entire pipeline. Or whatever this issue proposes to allow contrib to ignore these errors that are only triggered during automated testing and have no impact on runtime behavior.
Haven't closely read every comment, but this seems like the right issue to raise this concern. 😅 Please let me know if I should be barking up another tree, instead.
Thanks!
-Derek
Oh yeah, apologies about that. I forgot we ran into that at
📌
Fix: The update to convert "numeric" arguments to "entity_target_id" is deprecated
Needs review
, too. The only problem is the $strictConfigSchema
being set to TRUE
in this case. Everything still works, but the strict config schema checks will not in 10.2.x and earlier.
We can either disable those checks entirely, or conditionally for older test runs. See https://git.drupalcode.org/project/scheduler/-/merge_requests/159/diffs for one approach.
p.s. The failure is for "Previous minor", not next, which makes sense.
quietone → credited dww → .
Wow, blast from the past! Indeed, in D8+ ("modern Drupal"), when you're defining a block (Plugin), you can specify the #cache key and you get all sorts of control.
https://www.drupal.org/docs/drupal-apis/cache-api/cache-max-age → is still sorta broken. See the Limitations of max-age section → for details. But that's not what this issue was trying to solve.
Now that D7 is EOL and we're closing all the old issues, this one can die, too.
Thanks!
-Derek
In principle, this all sounds good.
Initial cleanup of MR metadata and resolving threads that have already been addressed.
Need to do a more thorough review, but no time for that right now.
Note: I see the D12 compatibility was added so that the upgrade_status job would "work" when run against 11.1.x core. This is the wrong approach. It's pointless to run the upgrade_status job on every MR pipeline until you're actually getting ready to port to D12.
Note: probably not wise to declare D12 compatibility until D12's API is finalized. 😅 See 🐛 Premature declaration of D12 compatibility Active