🇺🇸United States @xjm

Re: #24:

You have a logic error actually. 😀

All classes and all of their methods (including private methods but excluding constructors) must be documented.

This does not mean that no constructors require documentation. It only means that not all constructors require documentation.

Thanks @sourav_paul. That gives me some ideas to simplify it further.

As mentioned, I think we do not need to repeat the sentence that assertions are enabled in the title, value, and description. So for the description, let's drop that first sentence. How about:

Runtime assertions can negatively impact performance and are not recommended on production sites. Disable them in your PHP configuration to improve performance. Refer to the runtime assertion documentation for more information.

(Note also that I changed "see" to "refer to" for inclusivity.)

Hang on there... I think we can only delete constructor documentation when the class has members documented.

We don't need an upgrade hook necessarily; we've historically handled views data integrity issues along these lines in a gentle way where repair them on edit load or save.

By the time we get to the line in #35, Views needs to have already converted the empty string to a zero. That's what we need to fix. (Versus changing FormattableMarkup to silently accept bad input which is an even bigger and lower-level change.)

Adding all these integer typecasts every time the method is used in core seems suspect to me, and like it would hide errors in calling code. It seems like a bit of an antipattern. It also wouldn't address uses in contrib/custom code that have the same issue. And it would be a lot of mess to clean up in D12.

Why isn't the temporary BC layer we've added in QueryPluginBase::setOffset() plus the deprecation sufficient?

Tagging for subsystem maintainer review, although I cycled through "release" and "framework" a few times for the generic code maintainability/architecture questions, and just landed back on subsystem because committers will ultimately look at the issue again one way or another.

Just reiterating for posterity: We do not defer usability reviews to followups. Usability is a core gate → , and hard-blocking. 🙂 Very rarely we will grant an exception to mitigate data loss or the like, but in this case user expectations about what should happen are deeply intertwined with what the user considers data loss, so it is an essential part of the issue.

I remembered to get access to the PR before re-running the randomly failing test job so that I don't break the pipeline.

1. My first reaction on reading this issue was that this was babysitting broken code, but after thinking about it for a minute this seems like a valid usecase, e.g.:

   User input: This is my title!@#$#@$@

   Machine name: thisismytitle

2. Maybe we could add an additional test case illustrating the empty replace key, as above?
3. I also looked at the docs for the element to see what guidance they give us and if they need an update. They say:

   • replace_pattern: (optional) A regular expression (without delimiters) matching disallowed characters in the machine name. Defaults to '[^a-z0-9_]+'.
   • replace: (optional) A character to replace disallowed characters in the machine name via JavaScript. Defaults to '_' (underscore). When using a different character, 'replace_pattern' needs to be set accordingly.

   I think that still is all valid and covers the empty string case already?

4. Meanwhile, posted some small suggestions on the merge request.

This might want subsystem maintainer and/or FEFM review? But it seems pretty close to ready. Thanks everyone!

I've allowed this in practice, although there's a bit of a judgment call. If it's one docblock that's being touched within the scope of a single API, that's one thing. But if it's dozens, that's a lot of extra code to review to ensure that there isn't any mismatch between what's being removed versus what's being updated by the MR.

Do we need to amend the scope policy for this, versus just leaving it at the committer's discretion?

@smustgrave re: #31 I guess we can discuss over there and ask for FM input, but my initial reaction is extreme "yeahrghch". 😅

🐛 Cast values to string in placeholderEscape Active and 🐛 An empty views pager offset field can cause a PHP type error to be triggered. Needs work also make me think we need this fixed at the Views plugin level in two ways:

1. Converting empty user input to a plugin-appropriate empty value (zero or whatever).
2. On edit or save, handling existing null/wrongly empty-typed values and casting them to the plugin-appropriate empty value so the view isn't perma-fried because you paged your view by empty-string back in Drupal freaking 6 or something.

In the short term, we can fix it for just this plugin, because this specific bug is quite bad. In general, though, we should have a followup to fix this generically at a lower level in Views.

xjm → changed the visibility of the branch fix-3564838-adapt-placeholderescape to hidden.

Setting active as there isn't really a workable MR.

I think this needs to be fixed in Views instead. FormattableMarkup should absolutely not just cast any placeholder it receives to strings; that will hide all kinds of bugs and data integrity problems instead of properly reporting them to the developer.

Just actually read 🐛 Cast values to string in placeholderEscape Active and I think that's a wontfix. Instead, I think needs to be fixed somewhere lower in the Views plugin system, but Html::placeholderEscape() blindly casting things to strings sounds kind of evil.

Yeah, not sure if it should be combined or merely postponed, but we definitely need functional test coverage.

If this issue is only about the JS error, it's mis-titled, because the "WSOD" is the typecast problem and the major bug (possibly critical if it affects other elements besides this one).

Demoting back to normal. Unlike 🐛 Views Pager setting 'Items per page' with empty value gives WSOD Active , this one doesn't seem to permanently break the view, just cause an error in the UI and the log. (I did not yet manually test the paging behavior of the view itself, but the view is at least still configurable.)

I used the following steps to test this:

1. Install 11.x with the Standard profile.
2. Go to /admin/structure/views/view/comment.
3. Click "Paged, 50 items", blank out items per page, and click "Apply".
4. Observe the following error message:
   
5. Click save, and observe the WSOD:

6. The view is perma-borked and cannot be edited -- always a WSOD whenever I visit its edit URL from that point on -- so pretty major data integrity bug that allows this to be saved at all.

However, I manually tested with the diff from the MR applied and it did not appear to resolve the issue. I no longer get a console error in step 4 when I blank out the field, but I do still get a WSOD when I save the View in step 5 or try to edit it in step 6. The log message is still:

TypeError: Drupal\Component\Utility\Html::escape(): Argument #1 ($text) must be of type string, null given, called in /Users/xjm/git/drupal-test-site/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 238 in Drupal\Component\Utility\Html::escape() (line 433 of /Users/xjm/git/drupal-test-site/core/lib/Drupal/Component/Utility/Html.php).

I retried a few times, including the classic echo "hi"; to make sure the code was actually applied. 😉

This looks great and makes total sense, but it would be good to get @alexpott's signoff to be sure.

I committed the governance addition.

I think we probably want to add documentation to the core gates → (at least a stub section that can be filled in later)?

I went ahead and committed this since it was accepted by the Core Leadership Team through the decision-making process and has had broad consensus for months. Thanks everyone!

Posted some thoughts.

I get wanting to just fix the one instance, but this is a child/duplicate of 📌 Replace string 'path/to/class' with ::class constant Needs work . Adding your credits over there. Thanks!

Cherry-picked to 11.3.x as well now, and adding credit for @catch on review of the backport. Thanks!

Committed to 11.x, thanks!

It did not cherry-pick cleanly to 11.3.x, but I assume we want it there too?

The message was very developer-in-the-weeds-y for the average site owner and a bit redundant between heading and description, so I proposed an edit. NR.

It'd also be good to provide a screenshot of this warning in the user interface in context. The screenshot can be embedded in the issue summary and cropped to the relevant warning.

I agree that this doesn't need test coverage (although I guess it has it; that's fine too), but it should be manually tested to confirm that the warning goes away when assertions are turned off and appears when assertions are turned on.

Retitling to make it clear we're not "handling" them (which would be brittle and hide bugs). We're returning FALSE to trigger an exception in the caller. The original title had me all set to NW it but the approach is actually sound.

At first I was confused by this, because I was expecting the date parts to allow valid month formats like "January", but it does indeed expect it to be integers.

There's an argument to be made that checkArray() should be a protected method, for the reason in #17.

Since int is expected, I am not sure is_numeric() goes far enough, since that could be floats and whatnot. It should be an int, or a string whose numeric value is an int.

I agree letting the caller handle throwing the exception is fine, and let's open a followup to discuss whether we should deprecate this being public and make it protected next major.

Thanks!

Getting mixed signals... The MR is still marked as a draft. :) If it is not a draft, please mark it as ready. :)

Maintenance minor releases do receive dependency updates, so 10.6 should also actually update to 2.9.2 itself. This means it presumably also needs the hunk that the 11.x and 11.3 MRs had?

Also, it looks like things are being done out of order here -- this was moved to 10.6 before 11.2 was addressed, which is going to create confusion even though yes 10.6 is bugfix while 11.2 is security-only. Since composer/composer is still just a dev dependency, there is a strong case to be made that updating it for 11.2 (and 10.5) is not an issue and we should commit it there as well.

So, can we get 10.6 to update the dep, and can we get MRs for 11.2 and 10.5? The 11.2 MR should be RTBCed and committed first so that committers don't get totally confused.

Thanks!

This seems like a good fix, but some refactoring has taken place. The method that does this now lives on a trait, and the class mentioned does not use the trait. Rather, a descendant of it does. We can either reference the child class or the trait. I made a suggestion on the MR using the trait, but feel free to use the child class instead if preferred.

Posted a few review points on this while d.o was down. The main reason it's failing is that some code was copied outside where some dependencies were injected into update hooks, so some services in the post-update were being used when they weren't available.

xjm → created an issue.

Committed to 11.x, and cherry-picked to 11.3.x as a patch-safe documentation fix. Thanks!

I also did a visual scan of the grep results without excluding those with Test in the namespace (i.e., the above grep without the last bit after the last pipe). There were a couple out of scope raised eyebrows, but once I realized it was splitting on _ things made more sense. I will be filing a couple followups for out-of-scope issues with other namespaces, but I didn't identify any incorrectly uppercased module names in test class namespaces on visual scan of the grep results.

I came across this MR randomly when d.o was down, having forgotten about the legacy issue, and thought "ugh that's ugly but also ugh changing it is going to break things". Basically what we all said on the issue ten years ago.

I am honestly afraid to change this away from the magic number 10 because I still don't know what awful downstream consequences it could have (WRT hook_module_implements_alter() arms races).

We need more specific steps to reproduce this issue, in a format like:

1. Install Drupal core 11.3.0 with the Standard Profile and Module X.
2. At admin/structure/views, create the following view....

Hmm. After thinking about this for a few minutes, I am also wondering if the default values for these labels are also not-the-best.

For dates, I think we should just ship default config of "Start date" and "End date".

Thanks for your work on this issue! Providing review both in my capacity as a committer and as a Views subsystem maintainer (and Views in Drupal Core initiative team member from back in 2012-2013).

1. We still need automated test coverage in the canonical MR. Was it maybe applied to a hidden branch by mistake?

2. I agree with the previous review that "Min label" and "Max label" are (ironically) not great labels. We generally don't use abbreviations like that in the user interface; it's against our content style guidelines. That said, in the screenshot in #18, we already have "Min placeholder" and "Max placeholder", so I guess Views is already violating the content style guidelines. (Not for the first time.)

   I can actually answer this:

   I didn't dig far into it, but it's been this way in the admin interface a long time since before Views was added to Core. I wonder if they ever underwent usability review.

   Several rounds of usability review, in fact, before both D7 and D8. But it was after the fact when Views already existed as a thing a developer built for developers. So, we had to prioritize. Views had so many features that we had to focus on removing as many elements as possible.

   In this case, I would recommend we come up with better, clearer labels for the fields that follow our content style guidelines, and then have a followup to fix the existing fields above it to match.

3. Speaking of usability review. In the old screenshots from back in 2022, there is a description on the fields that is redundant with the labels without adding any real information. We did a lot of work during Views in Drupal Core to remove these redundant descriptions from the Views UI. I don't see these descriptions in the schema in the MR, though, so are they being inherited, or is it a mismatch between the screenshot and the MR?

   If they are being inherited, it's worth either overriding it or a followup to fix the parent to not have redundant noise everywhere in the Views UI. Because if that snuck in somehow it undid weeks of VDC and usability team collaboration and that makes me sad.

   In either case, let's get updated screenshots to confirm the behavior, since it's been a few years.

Sorry, didn't mean to end with a rhetorical and d.o had a nap before I could fix it. :)

I suggest we would recommend at least 8.4, and communicate that in the release notes, platform requirement docs, etc.

We could also consider also having it on the core requirements listing programmatically as maybe a should- or could-have for D12 core to allow drivers to specify recommended versions.

We had an interesting discussion about this issue today while Drupal.org was down with @Moshe, @nicxvan, and @phenaproxima.

My perspective is that these methods are confusingly named and actually make UserInterface harder to understand without adding much value. They are wrappers for basic array manipulation that calling code should create its own callbacks for

@Moshe, who is a subsystem maintainer for user, made the point that:

my objection is that it encourages role checking. We want to encourage permission checking

This received positive reactions from other contributors in the Slack thread, including myself (a release manager) and @larowlan (a framework manager).

@phenaproxima also suggested that it might be better to provide code snippets demonstrating what the original poster was trying to accomplish. PHP 8.4 makes this cleaner and more readable than ever, mostly eliminating the readability problem from the original issue back in 2018. Quoting pseudocode directly from Slack:

array_any(['some role', 'another role'], $user->hasRole(...));

array_all(['role1', 'role2'], $user->hasRole(...))

Based on all that, I consider this issue wontfix.

Thanks everyone for your efforts so far. Even when we do wontfix an issue, work done on it helps us decide whether to include it or not, and improves Drupal's code quality and maintainability overall.

waggles fingers

We have precedent for the minimum required version of a platform requirement being below (even well below) what's EOL. See: Drupal 8 and PHP 5.3. It created serious problems later in Drupal 8's lifecycle, but it is a minimum.

Should we adopt recommended versions for databases like we have for PHP? It doesn't necessarily need to be enforced programmatically; could just be docs. And then what would we say the minimum supported version of MySQL would be, versus the recommended version?

Fix hyphenation. But really, maybe this page should be EOLed?

Fix page title casing.

Fix title casing.

Fix title casing.

Fix page title casing.

Fix page title.

Fix title casing.

I am not sure if we should make this change. While there are periodic SAs where some permission should have had restrict access: true and didn't, there are also cases where it is somewhat misused and covers less-than-desirable security design or hardening in a module or its permission architecture.

On the other hand, the change would be extremely disruptive, when the benefit is at best marginal.

If we did decide to make this change, I would put it on a two-major deprecation path. That is, if we landed a deprecation between now and 12.0.0, it would be deprecated for NULL to be disallowed in 13.0.0. If it landed in 12.1 or later, then the deprecation would be for 14.0.0.

But I'd prefer not to make the change, and instead pursue other ways of making developers think more carefully about their permission schemes, and maybe even consider separately the specific case of "dangerous modules" (things that let you do no-hold-barred things with JavaScript or that allow embedding executable code generally).

I'm also sensitive to the case of messaging fatigue, and agree that any solution should involve usability and accessibility feedback so that the warning is noticed, rather than ignored. For example, could we have a summary on the status report of non-administrative roles have restricted access permissions? That links to a dedicated permissions report with a summary? That could be a way of conveying the information that's not going to be just that same message over and over and over, scanned or clicked past...

xjm → created an issue.

Committed the hotfix to 11.x. Thanks!

xjm → created an issue.

Snark aside, I think this would be a serious regression in the review process that would add work rather than reducing it and am strongly -1. I agree with the previous remarks from @catch and @phenaproxima.

There are many, many automated tooling improvements we can invest in that would reduce workload -- not to mention direct investments in human reviewers -- that would actually improve the review process.

AI hallucinations with serious misinformation are a serious issue I encounter regularly, and "seemingly plausible but actually seriously wrong" is a very accurate description of most LLM-generated code reviews I've seen.

Reminder: Dries Buytaert is the founder of WordPress and associated with the internet.

xjm → credited xjm → .

Compared this again to the original commit from the original issue. Confirmed the only differences between cherry-picking the commit and the MR are in the lockfile hash and core-recommended:

[ayrton:drupal | Tue 19:46:18] $ git diff 3561574-bump-versions-11.2.x --stat
 composer.lock                                      | 150 ++++++++++-----------
 composer/Metapackage/CoreRecommended/composer.json |  24 ++--
 2 files changed, 85 insertions(+), 89 deletions(-)

I then reviewed those specific changes with:

[ayrton:drupal | Tue 19:46:37] $ git diff 3561574-bump-versions-11.2.x --color-words="."

I verified that the only differences are in lockfile hashes and timestamps, patch version increases to individual Symfony patches, and in one case an addition to the release managers listed as supporting a package in the lockfile. So those are all appropriate changes in a patch release.

Finally, I ran a composer validate --check-lock just to make sure the hashes etc. were correct, and they are, so RTBCing.

Edited my above comment to mention that we also bumped the priority of the open issues based on our best understanding of our severity. I should have also promoted this meta itself at the time, so doing that now.

It's also always helpful to have accessibility maintainers or experts in the field give feedback on the relative severity of the individual issues, and of course we could use help fixing them in the best way possible as well. Most accessibility bugs are eligible for backport even in patch releases, because the disruption to sites is outweighed by the value of resolving the accessibility bug. Resolving the more serious accessibility issues is our top priority for Navigation going forward.

Also no longer in a novice status. Thanks!

At DrupalCon Nara I told someone verbally this needed framework manager review, which somehow never made it back onto the issue, but looks like @alexpott found it anyway which is good.

I went ahead and committed the 10.5.x MR despite the normal backport policy. In some ways, the 10.5.x hotfix release is more urgent, since 10.5.x sites are more in need of security-only dependency management and less likely to want to simply update Symfony to the latest release for the sake of updating it.

NW for the 11.2.x fix to remove the minor updates of those two components which must be slipping by indirect constraints or something. (The patch-level updates are fine.)

Reassuringly, the branch I created by cherry-picking the original commit to 10.5.x differs from the new MR that was created above by only:

diff --git a/composer.lock b/composer.lock
index d8cf1dc814e..339476ca045 100644
--- a/composer.lock
+++ b/composer.lock
@@ -10211,5 +10211,5 @@
     "platform-overrides": {
         "php": "8.1.0"
     },
-    "plugin-api-version": "2.6.0"
+    "plugin-api-version": "2.9.0"
 }

I made a similar cherry-pick on 11.2 for comparison. That diff includes different versions of numerous Symfony packages than what we had before. To some extent, this is to be expected; however, I did notice that we're upgrading the minor version from 7.3.0 to 7.4.0 of symfony-string and symfony/var-exporter. That shouldn't be happening in a patch release.

Sorry I have no idea what diff I was looking at that had only lockfile changes and nothing else. Please disregard above several comments (aside from the 10.4.x cherry-pick which is fine).

It's interesting that no tests are failing despite the dependencies in composer.lock not matching what's in the metapackages in the above MRs.

Probably we should just try to follow the documented steps for updating core Composer dependencies → for at least HttpFoundation.

I addressed 10.4.x by cherry-picking the original fix from 📌 Update Symfony to 7.3.7 / 6.4.29 Active and applying it again. That worked since there were no commits on 10.4.x since the release and none of the other changes in the security release touched the metapackages or lockfile.

Sorry, I think this is actually NW. There should be a corresponding increase in the constraint of http-foundation in core-recommended to the secure version. 10.6.x has this, and these branches also did prior to the merge commit eating it. See 📌 Update Symfony to 7.3.7 / 6.4.29 Active .

One thing that's interesting is that these are lockfile-only issues. Did we not increase constraints for the Symfony updates last month?

@catch and I discussed this hotfix. We agree it can be committed with the upgrade tests as a followup if the upgrade has been manually tested so that we confirm the issue is actually resolved. It should be tested both on a site where the update already broke and on a site that has not yet updated. Thanks!

I added credit for Rachel who also helped us a bit remotely with some of the organizational aspects, and for richo_au who contributed significantly to the event planning for the contribution day.

Thanks everyone!

10.6.x should be fine because it did not have a merge commit, just a forward-port.

However, I just checked and 11.2.9 has the same issue. It installs with 7.3.0, while 11.2.8 installs with 7.3.7. We definitely need an MR for 11.2.

(11.1 probably also has the issue, and we can ignore it like 10.4, but 11.2 needs the hotfix.)

@longwave correctly pointed out that 10.4.x is end-of-life in about 6 days, so we probably don't actually need to make a 10.4.x merge request because we're unlikely to create another 10.4 release unless some highly critical core security issue comes up that we would backport outside normal policy. Hopefully that doesn't happen. 😅 If that happens, or if we backport some other upgrade path fix to 10.4.x for some reason, then we can fix this, but otherwise 10.5.x itself is enough for now. Thanks!

Critical was an overreaction on my part because I was annoyed with myself; downgrading to major. While this is a serious bug from a release process standpoint, it is not a security issue. The site owner can still update the Symfony dependency easily -- it is only a patch-level update -- and Drupal itself isn't even vulnerable to the issue.

I just checked and while 10.4.x has not had a release since 10.4.9, it looks like it has the same lockfile merge issues that 10.5.x does. This means that the next time a 10.4.x security release is changed, it will revert the Symfony update. So we will need an MR for 10.4.x as well.

I think this is due to an issue with the security release tagging script naively assuming that security updates do not change the lockfile beyond the version constant. So while we resolved the new issue that came up with the new attempted process tags themselves during the security window, we did not resolve the second, similar, existing issue with the "Back to dev" commit.

In the longer term this might require a change to the tagging script in order to create proper "Back to dev" commits, or at least a manual workaround by the release manager to amend the commit prior to pushing branch endpoints after the window.

Pipeline failure was:

    1) Drupal\Tests\system\Functional\System\CronRunTest::testCronUI
    Cron does not run when saving the configuration form.
    Failed asserting that 1764887928 matches expected 1764887733.

Sounds random, so I requeued it.

This may have been due to an error during the tagging process that was corrected for the other branches but possibly not for 10.5. We should double-check to ensure that 10.4 doesn't have the same issue.

Added attendees to the contribution record. Thanks!

xjm → created an issue.

Assuming the downgrade was a crosspost. :)

This kind of sort of should have been a stable blocker.

The documentation page should actually be provided at https://www.drupal.org/docs/develop/core-modules-and-themes/core-modules... → now that the module is in core.

Navigation is stable!

📌 Register equivalent updates on site install Active would be helpful to fix before 11.3.0 and 10.6.0 if we can (backporting anything necessary to 11.2.x and 10.5.x patch releases as well as needed).

xjm → created an issue.

greggles → credited xjm → .

xjm → credited xjm → .