undefined - Contrib.social

🇺🇸United States dww

Hate to say it, but I think this might still be valid. From what I understand, local still has some dependencies on update module. There are also issues to OOP-ify Update module, which this is related to as well. Eg a class to represent project info, not the monster arrays we pass around now.

📌 | Drupal core | Replace hook_requirements with tagged services

🇺🇸United States dww

FYI: Both update and runtime are RTBC. There will be a merge conflict with the 2nd one after the 1st one lands, since both are touching core/lib/Drupal/Core/Extension/module.api.php -- it'll be a trivial fix, but it means a core committer cannot do both simultaneously without one of us doing a manual rebase in between.

If we wanted to arbitrarily pick one to commit first and mark the other postponed, that'd be okay. Or we leave both RTBC and resolve it organically.

Thanks!
-Derek

📌 | Drupal core | Create class to replace hook_update_requirements

🇺🇸United States dww

Minor formatting for title and summary.

🇺🇸United States dww

p.s. Did a light edit on the CR for formatting and pedantic nits. 😂 https://www.drupal.org/node/3490851/revisions/view/13843726/13854897 →

🇺🇸United States dww

Phew! Thanks for sticking with this through all the rounds of reviews. Apologies I didn't spot everything all at once.

Everything is ready here now. Pipeline is green. Code and docs have been polished so much they shine. 😂 No other improvements I can spot. Per the meta, these separate hooks really are the right approach for splitting up the beast of hook_requirements().

Ready To Be Committed. 😉

📌 | Drupal core | Add headroom to the navigation performance test

🇺🇸United States dww

Thanks!

📌 Use PerformanceTestTrait::assertMetrics() in the navigation performance test Active

📌 | Drupal core | Use PerformanceTestTrait::assertMetrics() in the navigation performance test

🇺🇸United States dww

dww → created an issue.

🇺🇸United States dww

Blocker is in. This needs a rebase and to revert the last commit.

📌 | Drupal core | Add headroom to the navigation performance test

🇺🇸United States dww

Per Slack, catch is okay to commit this now and do the conversion in a follow up. I or nicxvan will open that follow-up momentarily, but catch only has 29 minutes to connnit this m, so here’s a quick RTBC. There’s a pipeline in the linked issue that shows this change allows that to pass. This is a trivial change. Ready!

Thanks,
-Derek

🇺🇸United States dww

Yeah, I don’t know if the quick-fix here in an unrelated test is the right approach. I believe we should postpone this on resolving 📌 Add headroom to the navigation performance test Active

📌 | Drupal core | Add headroom to the navigation performance test

🇺🇸United States dww

Thanks for that pointer, @catch. Let’s see what we can do to make this stable again.

📌 | Drupal core | Create class to replace hook_update_requirements

🇺🇸United States dww

I have a very minor lingering concern that INFO and OK are weird for update. They’re never displayed unless there’s an error or warning, in which case all update requirements, even OK and INFO, are shown to the user, distracting them from the thing they actually have to fix. 😕 This is not a problem to be solved in this issue. 😅 Maybe some of it can be addressed at 📌 Deprecate REQUIREMENT_INFO in favor of REQUIREMENT_OK Active , but probably we’ll want yet another follow up for the UX of update.php when there are requirements problems.

That said, I think we’ve done everything we can to make this a clean and solid improvement. RTBC!

Thanks,
-Derek

🇺🇸United States dww

This, too, might be RTBC, but lemme do it right if true.

🇺🇸United States dww

That said, I think we’ve done everything we can to make this a clean and solid improvement. RTBC!

Thanks,
-Derek

📌 | Drupal core | Deprecate REQUIREMENT_INFO in favor of REQUIREMENT_OK

🇺🇸United States dww

Upon further investigation, OK and INFO sort of matter in the update case, but even that's perhaps another UI problem to solve. See summary for details.

📌 | Drupal core | Deprecate REQUIREMENT_INFO in favor of REQUIREMENT_OK

🇺🇸United States dww

dww → created an issue.

📌 | Drupal core | Create class to replace hook_update_requirements

🇺🇸United States dww

Apologies, I missed a point in my previous feedback on the API docs.

🐛 | Drupal core | Double encoded ampersand in /admin/reports/updates

🇺🇸United States dww

Thanks for this bug report, and for the work to try to fix it. Per #19, this issue is duplicate with 🐛 Module names on the Available Updates page are double-escaped Needs review , which started doing much the same thing here, but then ended up moving to another queue since the problem is really in the XML release history feeds provided by updates.drupal.org, not the update.module itself. I just replied there in support of fixing this at u.d.o, not in update.module. Let's continue in the original issue. If it turns out we have to fix this only in update.module, we can move that issue back into the core queue.

Thanks again!
-Derek

🐛 | Drupal core | Module names on the Available Updates page are double-escaped

🇺🇸United States dww

Coming here from 🐛 Double encoded ampersand in /admin/reports/updates Active which I was pinged about in Slack for a subsystem maintainer review.

From a little Git archeology, looks like this double-encoding was introduced in #1003764: Add an alter hook invoked while generating release-history XML files → when @drumm converted this from manually putting the XML together into using SimpleXML. Apparently no one noticed that SimpleXML is doing another round of encoding for us.

I just did this:

% wget https://updates.drupal.org/release-history/save_edit/current -O save_edit.xml
...
HTTP request sent, awaiting response... 200 OK
Length: 9702 (9.5K) [text/xml]
Saving to: ‘save_edit.xml’

save_edit.xml             100%[=====================================>]   9.47K  --.-KB/s    in 0s

2025-01-15 14:45:23 (23.8 MB/s) - ‘save_edit.xml’ saved [9702/9702]
% grep title save_edit.xml
<title>Save &amp;amp; Edit</title>

So the source is definitely double-encoded. It seems wonky to say that core should have to decode twice to get the raw values.

I don't understand the "BC" concern here. The primary client for these feeds, update.module, was written to assume properly encoded markup in the XML feeds, and is now "broken" by the double encoding. If other clients are already decoding twice, and we stop double encoding, what's the harm?

Here's a little test script:


$inputs = [
  'not_encoded' => '<title>Save & Edit</title>',
  'encoded_once' => '<title>Save &amp; Edit</title>',
  'encoded_twice' => '<title>Save &amp;amp; Edit</title>',
];

foreach ($inputs as $key => $value) {
  echo "------\n$key\n";
  echo "Raw: $value\n";
  $decoded = html_entity_decode($value, ENT_QUOTES | ENT_HTML5, 'UTF-8');
  echo "Decoded: $decoded\n";
  echo "Double decoded: " . html_entity_decode($decoded, ENT_QUOTES | ENT_HTML5, 'UTF-8') . "\n";
}

If I run this, I get:

------
not_encoded
Raw: <title>Save & Edit</title>
Decoded: <title>Save & Edit</title>
Double decoded: <title>Save & Edit</title>
------
encoded_once
Raw: <title>Save &amp; Edit</title>
Decoded: <title>Save & Edit</title>
Double decoded: <title>Save & Edit</title>
------
encoded_twice
Raw: <title>Save &amp;amp; Edit</title>
Decoded: <title>Save &amp; Edit</title>
Double decoded: <title>Save & Edit</title>

We're still going to turn around and re-encode this when printing it in the available updates report. That happens via core/modules/update/templates/update-project-status.html.twig right here:

<div class="project-update__title">
  {%- if url -%}
    <a href="{{ url }}">{{ title }}</a>
  {%- else -%}
    {{ title }}
  {%- endif %}
  {{ existing_version }}
  {% if install_type == 'dev' and datestamp %}
    <span class="project-update__version-date">({{ datestamp }})</span>
  {% endif %}
</div>

So Twig is going to escape this for us, and I don't see how this could be used as an attack vector.

IMHO, this is the right fix, not the extra decode being proposed at #3493742. I am a maintainer for project*, so I could just commit this, but I can't deploy the fix on d.o anymore. 😅

🇺🇸United States dww

Needs rebase after 📌 Add array return to all hook_requirements implementations Active landed. Also, added a bunch of suggestions to cleanup the API docs.

Initial pass at saving credit: @nicxvan for all the work, and @smustgrave and myself for reviews.

💬 | Duration Field | Offering to co-maintain Duration Field

🇺🇸United States dww

Hi,

Thanks for your interest in this module. However, I've never seen an issue from you, a patch or MR from you, or even a comment in this issue queue that you're using this module and helping to test / fix anything. I have no idea who you are, if you know how to write secure code, if you know how to write good tests, if you know how to manage releases, database schema updates, etc. There's no way I could possibly grant you access to push commits to this project, much less "co-maintain" it since you've never contributed anything to it and I have no information about you, your experience, abilities, or anything.

Also, if you have a "strong team" that is "managing" this module, I'd love to see some evidence of that in the issue queue before you request for your "team" to take over. Note that drupal.org works with human accounts, not teams. If you have a team, and they're all helping, we need to see activity from each member of that team who will would potentially become a maintainer. Sharing accounts or credentials among a team is disallowed by drupal.org policy.

In the future, if you and your strong team want to contribute to the modules hosted here, I strongly encourage you to start by actually contributing to the module in any/all of the following ways:

Triage the issue queue (look for duplicates, make sure bugs can be reproduced, that issue summaries are clear and accurate, etc).
Help review and test existing merge requests.
Write test coverage for MRs that are lacking tests.
Write new merge requests for issues that do not yet have code but need it.
...

If you perform any (ideally, many) of these tasks, you will begin to earn some respect and trust from existing maintainers. Only after a period of building respect and trust would any responsible maintainer seriously consider granting anyone else access.

Cheers,
-Derek

🐛 | Drupal core | Disallow saving the current default revision as a non-default revision

🇺🇸United States dww

Maybe postponed is more appropriate, but with known-failing tests, this is NW. Super minor nit as an MR suggestion.

📌 | Drupal core | Deprecate views_entity_field_label() in favor of a class method outside Views

🇺🇸United States dww

Pipeline is now green.
All feedback addressed.
All threads resolved.
Title and summary are accurate and clear.
CR is simple, accurate and clear.

I see nothing left to improve. RTBC.

Thanks!
-Derek

📌 | Drupal core | Deprecate views_entity_field_label() in favor of a class method outside Views

🇺🇸United States dww

p.s. I did some minor edits and formatting cleanups on the CR: https://www.drupal.org/node/3489411/revisions/view/13800107/13851975 →

📌 | Drupal core | Deprecate views_entity_field_label() in favor of a class method outside Views

🇺🇸United States dww

Sorry, I missed something in previous reviews (or this is a new bug). 1 suggestion to apply, otherwise seems RTBC to me.

📌 | Drupal core | Deprecate views_entity_field_label() in favor of a class method outside Views

🇺🇸United States dww

Related, but the rebase seems to have clobbered views.views.inc entirely. I thought we still needed a deprecated views_entity_field_label() in there.

TL;DR: Probably wise not to self-RTBC after a rebase, especially with merge conflicts, so that peer review can help spot trouble. 😅

📌 | Drupal core | Deprecate views_entity_field_label() in favor of a class method outside Views

🇺🇸United States dww

phpstan is failing in the pipeline.

📌 | Drupal core | Clean up code in UserRolesCacheContext::getContext()

Comment 11 days ago →

🇺🇸United States dww

Thanks noticing this was requested in the parent issue, for opening it and for the MR.

Change is simple and small. Definitely improvement and cleanup. Issue summary and title are clear and accurate.

Pipeline is all green.

I see nothing to improve. Ready to be committed. 😉

Comment 12 days ago →

🇺🇸United States dww

The MR thinks the merge is blocked due to conflicts. Would you be willing to rebase? I just did a thorough review on the sister issue. Probably many of those nits would apply here, too. ;) I'm sadly out of contrib time for today, so I'll have to come back to this later for a more thorough review of the details here.

📌 | Drupal core | Create class to replace hook_update_requirements

Comment 12 days ago →

🇺🇸United States dww

Thanks for moving this forward. The parent meta has good discussion on why this is actually the right approach, after all. 😅

Left a bunch of suggestions on the MR. Mostly pedantic nits, but a few things of substance.

🐛 | Drupal core | Race conditions/bad lock logic in CacheCollector/State

Comment 12 days ago →

🇺🇸United States dww

(Sorry for the assigned/unassigned noise -- accidentally clicked something in the MR review UI).

Resolved the opened threads, since @catch has already addressed all the feedback.

Opened a few trivial suggestions for some nits, and to rename a test method that no longer does what the old name says.

I would RTBC, but I think the suggestions are worth applying, first. 😅

Initial pass at saving credit:
@catch (duh)
@alexpott for MR reviews + suggestions
@Spokje, @nicxvan, @godotislate, @luke.leber, and myself for MR and issue reviews

Almost there!

Thanks,
-Derek

p.s. restoring the Bug Smash Initiative tag, which I think @catch accidentally clobbered in #13 with a stale form submit.

📌 | Drupal core | Deprecate views_entity_field_label() in favor of a class method outside Views

Comment 13 days ago →

🇺🇸United States dww

Thanks, looks better.

Issue title and summary look clear and accurate.

Saving credit for quietone and myself for MR reviews.

Don’t see anything else to improve. RTBC++

📌 | Drupal core | Deprecate views_entity_field_label() in favor of a class method outside Views

Comment 13 days ago →

🇺🇸United States dww

Added some suggestions for very minor / pedantic nits. Otherwise, seems reasonable to me.

🐛 | Drupal core | [random test failure] ImageStylesPathAndUrlTest

🇺🇸United States dww

I was thinking that if release managers are saying “we shouldn’t use state in tests, only keyvalue, that we could do bulk.

But yes, strongly appreciate the long time it takes to do even single tests carefully. Wouldn’t dream of accusing you of lots of issues for credit. You’re a prolific contributor!

I was only wondering if we could add more scope without as much effort and rigor if we want to convert entirely. It’s a bit weird having a mix of both, so I’m hoping to minimize the time it takes to convert.

All that said, yes, following the issue where the underlying race condition in State API is hopefully going to be fixed. So maybe it’s moot to do this conversation?

Yeah, I’m torn, I guess we’ll leave it for the core committers to decide. Back to RTBC.

Thanks!
-Derek

📌 | Drupal core | [pp-1] Mark several more modules as converted

🇺🇸United States dww

📌 | Drupal core | Update move procedural hooks and mark as converted

🇺🇸United States dww

Sorry, totally missed. I had a stale copy of a branch checked out, and was only looking at the MR diff which seemed thin for some reason.

So we've already moved everything we can move, and converted everything to UpdateHooks, etc. The title here is kinda misleading. Maybe this is closer? I know you opened these issues in bulk with copy/pasta summaries and titles, but I think it's worth refining before it ends up in the Git history.

p.s. Saving credit to nicxvan for the MR, bramdriesen and myself for reviews, but not the patch in #3.

🐛 | Drupal core | Race conditions/bad lock logic in CacheCollector/State

🇺🇸United States dww

Thanks for this! Great work tracking this down.

I added some suggestions to the MR, mostly for trivial nits and a typo. Hate to slow down progress on a critical, but I really think we need comments about why we're not releasing the locks we're acquiring. I don't think this is RTBC if all the MR threads aren't resolved.

🐛 | Drupal core | fatal error on `update` module enable on composer-managed D11.1.0 instance

🇺🇸United States dww

See comment #15. It’s being set to the translatable string “Unknown” but then being treated like an actual version string, which we fail to parse.

It’s a problem that this user’s site has no version info.
It’s a bug that in this case, update.module now does fatal errors, instead of gracefully handling the problem.

🐛 | Drupal core | fatal error on `update` module enable on composer-managed D11.1.0 instance

🇺🇸United States dww

Disclaimer: it's a bug in update.module* that the error case for an unknown version string ends up as a fatal error. We should probably fix that, regardless of why your site has no version info for core.

Meanwhile, the most useful things for me to understand this would be:

The entire contents of core/modules/node/node.info.yml on the site where this is happening
The VERSION const from core/lib/Drupal.php

Looking at the output posted above, I notice:

composer require drupal/redis:dev-1.x

Which means you're asking for a "dev tarball". For that case, I'd also love to see:

The entire contents of modules/contrib/redis/redis.info.yml
If modules/contrib/redis/.git exists. Is it a git checkout?

To add another variable to the mix, if you're using a Git checkout of any part of Drupal (core or contrib), you need to use git_deploy → for update.module to work. I'm less familiar with composer_deploy.

📌 | Drupal core | Update move procedural hooks and mark as converted

🇺🇸United States dww

It doesn't look like we're actually converting anything here, right? We're just adding the attribute for when to stop scanning for procedural hooks in a bunch of files. Do we actually want to do some conversion, instead? Isn't that part of the fun of having split these out into smaller issues so we can make bigger changes where needed?

🐛 | Drupal core | [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing

🇺🇸United States dww

Excellent work here!

After checking out the issue fork branch, there are still a ton of references to \Drupal::state() in core/modules/block/tests/src/*. Do we want to fix everything? I'm not sure why we're converting some to keyValue and leaving others as state.

🐛 | Drupal core | [random test failure] ImageStylesPathAndUrlTest