🇺🇸United States @xjm

benjifisher → credited xjm → .

smustgrave → credited xjm → .

benjifisher → credited xjm → .

larowlan → credited xjm → .

mradcliffe → credited xjm → .

Ahh, thanks @lendude, I missed #6:

I think the collapsed “Advanced” column was a rather brute-force way to reduce clutter in the Views UI. I wouldn't mind showing it expanded again.

xjm → credited xjm → .

xjm → credited xjm → .

Tricky...

Committed to 11.x and published the change record. Thanks!

I made some small edits to the CR, and did one final round of manual testing.

Committed to 11.x and published the CR. This is only the second issue I've ever committed with a five-digit node ID, and the first I've ever committed with killes attributed. (For those that don't know, killes was the Drupal 4.7 release manager.)

I'm tagging this for the release highlights as it could be a nice bullet in a list of various site builder and content author UX improvements for 11.3.

Great work everyone!

Saving nearly 20 years' worth of issue credits. (‼️)

(Just adding tags. This was a proposal by @lauriii, but would need signoff and agreement from all the relevant stakeholders to move forward, including release managers, other product managers, and the node subsytem maintainers.)

Did some work to clarify the summary, including identifying some of the relevant default config.

The main problem with the search was actually not being able to get a string literal match for the underscore, even when I dropped the parenthesis. But anyway, it's fine with a D13 deprecation. Thanks!

I made a couple small fixes to the CR and fixed a broken link in the MR.

@rudi teschner, you should probably follow some of the steps above to diagnose what exactly is causing a Composer conflict, and you might need to either file a separate issue or make some changes to what you have locally. Thanks!

Reviewed locally with git diff --staged --color-words just to confirm that there weren't any changes other than the added t() calls, the capitalization, and the periods:

Since it adds new translatable strings for the other translations, I've committed it to 11.x only. Thanks everyone for working on this and congratulations @anapaulagoetze on your first core issue credit!

Sorry I keep trying and failing to remove the tag; crossposts and d.o timeouts. 😀

Thanks @uesliI think we're good on manual testing now since @himanshu raj's tenth screenshot appears to be with the MR applied and a confirmation of my result. The last thing we need for this to be RTBC is a general review (code review, confirmation that the changes meet Drupal style guidelines and translation best practices, etc.) Thanks!

@mstrelan, @acbramley, Great work here. Sorry for not being able to follow up and work on this more. (I only have 8 hours of paid time per week because I lost most of my sponsorship.)

Thanks @himanshu raj. For next time, please embed your screenshots in your comment (see mine above for an example), and indicate which of your screenshots are before the MR and which are after.

Also, we don't need to know that the code compiled without errors or need screenshots of your IDE; the automated testing tells us that. 🙂 thanks!

I tested manually, and confirmed that the two strings that already exist in core are now automatically translated, and that the rest follow our content style guidelines. Nice work!

As an aside, I learned that the Russian translation for "header" is "шапка", which means "hat", and I find that absolutely delightful.

@himanshu raj, your screenshot in #17 seems to be without the patch applied?

I agree that it does seem very niche, but since it is a very dynamic public API that could be used in a lot of ways and I couldn't get a good sense of how many might still exist from the contrib search, I think I'd still deprecate it for Drupal 13 to be safe. .module files themselves aren't going to be removed before then, either, so it shouldn't be problematic to keep them around the extra two years. Thanks @mstrelan!

Committed to 11.x despite the BC break, based on my assessment in #27. I also backported it to 11.2.x in accordance with 📌 [policy, no patch] Update allowed changes to current practice for backport of Test API changes Active .

Thanks everyone!

Committed to 11.x. I also backported it to 11.2.x without the enabled PHPCS rule as follows:

[ayrton:maintainer | Sat 11:37:23] $ git checkout 11.2.x
Switched to branch '11.2.x'
Your branch is up to date with 'origin/11.2.x'.
[ayrton:maintainer | Sat 11:38:06] $ git pull
Already up to date.
[ayrton:maintainer | Sat 11:38:33] $ git cherry-pick -x 11.x
# ...
[ayrton:maintainer | Sat 11:38:58] $ git commit --amend -m 'Issue #3515661 by quietone, joaopauloscho, xjm, charlliequadros: Fix '\''DocComment.MissingShort'\'' in FunctionalJavascript tests'

Thanks everyone!

Saving credits from the contribution day.

Thanks @jonathandealmeida!

I think this means we would need to completely change how the test would work. Based on the machine name, the whole point is that the role does not exist. However, if we're conditionally creating it, that's no longer the casein the situation under test. In other words, fixing it in this way would basically remove the existing test coverage for non-existent roles.

This test coverage was added originally in #2737719-71: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method → . Quoting that comment:

Apparently another fail sneaked in there. In all Block config entities. The root cause is that same problem that was discovered before (see #41):

    // All blocks can be viewed by the anonymous user by default. An interesting
    // side effect of this is that any anonymous user is also able to read the
    // corresponding block config entity via REST, even if an authentication
    // provider is configured for the block config entity REST resource! In
    // other words: Block entities do not distinguish between 'view' as in
    // "render on a page" and 'view' as in "read the configuration".

But #65 introduced additional assertions to ensure a 403 is returned before the necessary authorization is set up. The work-around I had in place didn't account for that. So, now I have changed the work-around to disallow access by any user until the necessary permission is granted. We still need to fix the root cause though.

Created an issue for that now: #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity" → .

Adding those two issues to the related issues.

Based on the above, I don't think we can go ahead with the workaround of conditionally creating the role. Or, if we did, we would need to test the 4-3s for non-existent roles in some other way.

This is also definitely not novice anymore, so untagging.

Thanks @anapaulagoetze! Excellent writeup.

I followed the same testing steps with French, and I noticed that different variations of the same strings already exist. For example:

The current string has:
the number of items per page

But core already has this existing string:
The number of items per page.

If we update the merge request to match this (capitalize it and add a period), then the existing string translation will just start working here, without translators needing to do additional work. This would also better match our content standards. The same is also true for at least the total page count if we change it to The total page count.

So, I think we should also update the merge request to capitalize all the strings and add periods to the end of them. Thanks!

@uesli, I think from the screenshots #62 is testing Drupal 11.2 without the merge request, correct?

@liam morland That could be done elsewhere -- and there are meta issues to address missing typehints in core generally -- but it is out of scope for this issue, which is just about the one specific method. Thanks!

Thanks @joaopauloscho! Those changes look good to me. I made a couple small fixes to add the article "a" where it was missing.

I am leaving the issue at "Needs review" for now for another person to confirm the review. (That way I can still potentially commit it.)

Regarding:

I wonder if it is worth an issue to add a flag to phpstan drupal to ignore non next version deprecations since we're going to be getting a bunch of 13 removal deprecations now.

I believe Gábor confirmed that this is already taken care of in our upgrade tooling (although I can't say for sure if it was for phpstan-drupal specifically or just in e.g. Upgrade Status). This is one of the things we discussed before changing the policy, for sure.

@godotislate Ah, yeah, the internal API call should probably only do what it says on the tin and not vary according to the logged-in user account or anything like that. So changing things earlier in the callstack seems like the right approach. We might not be able to stop people from making bad out-of-context direct API calls with Drush; that is a Drush "feature" I guess.

In either case, we should manually test it carefully.

NB: I filed this as postponed and self-assigned because I may have someone look at it tomorrow.

xjm → created an issue.

Thanks @lauriii and @berdir.

I am okay with a followup for my suggestion about changing the default of pages, and a bigger followup for #63 makes sense as well. Once those followups are added this can be moved straight back to RTBC.

Thanks!

The screenshots appear to be in #33, but they also don't actually seem to show the "after" case. It's probably best not to embed them in the IS in either case, but we would want to at least link them. Tagging for new screenshots.

I disagree that this is a bug. It was a design behavior and was changed on purpose from a previous setup where the column was always shown, because we wanted to make the Views UI less overwhelming on page load. Therefore, I think this should have both usability and subsystem signoff. Once the screenshots are added and embedded in the IS, this can be tagged for usability review.

Thanks for the update!

Despite my earlier comment: This is pretty low-level, and the approach has changed several times over the issue's 13-year (!) history. So, I'd like subystem and/or framework signoff for the fix. Leaving RTBC for that.

Should we also change the default form display of these for the page content type on new sites? (Meaning, change the default config, but no upgrade path, since once the content type is installed, it belongs to the site.)

It probably makes sense to leave article as it is regardless since the front page promotion is one of its "features".

I was going to ask for product signoff, but #33 seems like that at least indirectly. However, since we've changed approach since then (to something much lighter weight and more BC), I'm going to confirm that @lauriii is okay with the new approach.

Agreed on not adding a lengthy description; that would be a usability regression. (We already spend a lot of time deliberately removing as many descriptions as possible from core fields, so let's not add more.)

We should generally include both before and after screenshots, and embed them in the IS.

Sorry, I should have provided the reference to the BC policy regarding test code → . In this case, since it's a method on a base class, it's borderline, but as a release manager I have discretion to allow internal BC breaks in certain cases like this one. :) Thanks!

The method is missing its parameter documentation, which does not help the situation. Given that we're changing the allowed values of the assertion method, I think it's also in scope to add parameter docs.

Since the current version is only broadening the allowed values of the method, it's pretty BC. That said, since we're talking of typehints, I think we can also expand the scope to add signature typehints to the assertion, which is an allowable BC break for test-only code. I checked contrib usages and overrides of this assertion, and there's only one contrib module in a non-D7 test: MinisiteTestBase.

Thanks everyone!

Helps if I remember to push the commit, oops!

Thanks @immaculatexavier. #8 is definitely a good way to evaluate this issue.

Previously, we would have added this to the release notes for the minor since it's enabling a new PHPCS rule, but we stopped doing that except in extreme cases. In the case of this rule, it is a very small addition (as can be seen from the fact that core already passes).

Committed to 11.x only as an addition to the core ruleset. Thanks everyone!

This change makes total sense to me. Almost all the major sites I've built had external authentication and so I was constantly altering them to kill the stupid link.

Couple very small things on the MR.

@catch proposed a followup in #18, but I actually do not think we should add a configuration option for this. Simply controlling the display to the link based on access to it is a well-established core pattern. Customizing the behavior outside that is something we can leave to contrib/custom code IMO.

Do we need to worry about cache variation for this?

I can see adding one of these new formats, but adding three of them is essentially almost doubling the number of date formats. Perhaps we should just pick one to include? I find the new list kind of overwhelming. I think this would need usability review in addition to product signoff. (Meaning, we would want to add before-and-after screenshots and then tag it for usability review.)

Also, given that these are only being added to new installs, but we are also changing the production Twig template for the announcements feed, that could be a problem, no? So that might be a reason we actually would need an upgrade path, at least for the one that Announcements is now expecting to exist. No?

#11 does indeed also sound like something we should confirm or revisit. It might be worth a git log -L on the comment to see why the issue that added it made the choice to differentiate the UI versus API deletion in this way. (There might be useful discussion on the original issue.)

Thanks @godotislate!

This will also eventually need usability review on whatever proposed solution we fix, and should be fixed in a way that's consistent with these other "config dependencies eat my config" issues.

Also, bumping to critical; nothing should be able to delete a storage in this way without an explicit user decision. When we uninstall a module, for example, the user has to do a bulk operation to remove module content before they can uninstall it.

Oops, xpost. Reviewing #11 now.

This prevents the data from deleted, but there is a display issue, in that when the format is deleted, existing content with text fields using that filter format can not display the content. However, this can be addressed by editing the entity and setting the filter format to something else.

This is also kinda problematic, replacing data loss with data integrity problems and/or "perceived" data loss.

Thanks @nicxvan.

Isn't one of the major benefits of moving disruptive deprecations to 13 so that issues like this can be incremental?
My understanding is that people should not even get warned about this deprecation until 13 is the next major. (aside from the CR, but we could add a note that it's a 13 removal with planned work to tweak the api's so keep an eye on future changes), this means the timeline for us to get the improvement @longwave is concerned about is when people start looking at upgrading to 13, not 11.3.

This is one of the points @catch, @longwave, and I discussed regarding the scoping of this issue, and our concern was that having an intermediate public API is non-ideal, especially with somewhat concerning DX. Attentive contrib maintainers would get two separate deprecations and update their modules twice, which is not what we want. It would be better to know the DX endpoint we're aiming for, rather than de-scoping that. All three of us had concerns about doing this in two steps.

Regarding point 2, in general, diffs of this size should be reserved for e.g. 1:1 replacements (not 3:9 replacements) because reviewers get fatigued and miss stuff when there are multiple patterns like this. This is really adding three new APIs rather than just one. Also, as you recognize in #67, these are API additions to different subsystems and therefore have different stakeholders who should probably be given an opportunity to review them.

Regarding:

His comment refers specifically to readability, that is not the goal of this change. The goal is to deprecate globally defined constants in system.module. The improved readability will come in the follow-up issues that change API to use the new enums properly.

Yes, that is what I meant and the point stands. Deprecating constants in system.module is not, in itself, a goal that surpasses a properly used and designed API in importance. ;) In general, changes that regress developer experience should only be made in major or critical situations. This is not that.

Hope this helps. Thanks!

I haven't read the solution in full yet, but it is extremely important that we block deletion of the filter format when stuff still uses it rather than anything else like altering config, since changing a filter format can have security implications.

Whatever solution should also have subsystem and/or framework manager review.

I read

</code> and confirmed that this is what it actually does. Since it's always good to check <em>why</em> something like this is wrong (especially to confirm that we have the correct replacement, I did:
<code>
git log -S "FullPageVariant"

It looks like this was removed back in 2014 in [#]. That issue includes e.g.:

diff --git a/core/lib/Drupal/Core/Display/Annotation/DisplayVariant.php b/core/lib/Drupal/Core/Display/Annotation/DisplayVariant.php
index 3c05cf3b85e..3bf74920f3c 100644
--- a/core/lib/Drupal/Core/Display/Annotation/DisplayVariant.php
+++ b/core/lib/Drupal/Core/Display/Annotation/DisplayVariant.php
@@ -25,8 +25,9 @@
  *
  * Plugin namespace: Plugin\DisplayVariant
  *
- * For a working example, see
- * \Drupal\block\Plugin\DisplayVariant\FullPageVariant
+ * For working examples, see
+ * - \Drupal\Core\Render\Plugin\DisplayVariant\SimplePageVariant
+ * - \Drupal\block\Plugin\DisplayVariant\BlockPageVariant
  *

...and git even detects it as a rewritten file:

 .../{FullPageVariant.php => BlockPageVariant.php}  |  12 +-

Finally, I grepped to confirm that there were no other stale references to FullPageVariant:

[ayrton:drupal | Fri 14:33:16] $ grep -r "FullPageVariant" *
core/lib/Drupal/Core/Display/PageVariantInterface.php: * For example, the \Drupal\block\Plugin\DisplayVariant\FullPageVariant page

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

defaultFieldImplementation() is too big as it is. If we want this code to be more testable, then it needs to be broken up. This is just one step toward doing that. No, it isn't strictly necessary from a functionality standpoint. But I will argue that it's necessary for sanity.

Should we have a followup to do the resT?

Posted some small stuff on the MR.

Meanwhile, is the IS up to date here? I didn't see anything in the patch that would result in the indicated UI. If it does, that's some serious magic. (I mean, it is Views, so maybe.)

Does this issue apply only to Views?

Finally, I think it would need a CR. Even if it's a bugfix, it may result in a behavior change in some circumstances.

My previous concern about this being quite long for a theme description remains. I don't know that it's the place of the theme info file to provide links to the theming documentation? I think we could drop the last bit with the link. The updated proposal could also get a quick usability review at the weekly usability meeting. To that end, a screenshot of the description in context alongside other themes' descriptions would be helpful.

Also, this still has not had subsystem maintainer feedback. However, given that the maintainer for this subsystem was out for the past eight weeks and is currently discussing their maintainership, we can escalate this to the FEFMs.

Fixing credit for me as the committer, which the bot ate.

Good catch, thanks @acbramley and @mstrelan!

I would ask if we should have a followup to add it back, but since this is a specific implementation of a larger API, it should be handled in our meta issues to add these typehints in a major version.

Committed to both 11.x and 11.2.x (separately, since it requires an updated baseline), as a hotfix for an unintentional BC break. It's also worth mentioning in the 11.2.3 release notes that we've resolved this regression.

Crossposted with the bot. No, bot. Bad bot.

We might want to target Drupal 13 for this deprecation. I tried to search contrib, but ran into problems with the fuzziness (it refusing to do an exact search for _cancel().

Nice find. This is an excellent cleanup, but we should probably have subsystem maintainer signoff to ensure we're not leaving behind any trailing related bits etc. I'm technically only a maintainer for Views, not Views UI, so bumping back to NR. I'll also ping Lendude.

It's not module tests; it's test modules.

OK, done reviewing now. Phew, this one is massive! Given the 290 total LOC diffstat of all new docs that furthermore requires you to read not only the context lines of the method but also entirely separate code of the tests these fixtures are for, I think the scope of this is too big. There are two particularly problematic tests that I think should be moved to their own separate issues. Thanks!

There's an example of a relevant test case that we should add in core/modules/system/tests/modules/test_page_test/src/Controller/Test.php -- see the escapedScript() and unEscapedScript() methods on that fixture. We should probably add an additional pair of test cases for (e.g.) escapedScriptWithoutQuotes() or something.

Midway through my review. The hook order tests need so much work that IMO we might want to pull them out into their own issue, especially since this is one of those test fixture sets where understanding exactly what is under test is important.

NW for small things... fortunately, this one is a nice small scope. Thanks!

Is there a particular reason to bump this by two patch versions mid-cycle? Bug fixes we want or suchlike? It does not seem to substantively affect the baseline, just that one hunk.

NW for that. Thanks!

@longwave and I are both release managers, so we do have discretion over issue scoping.

My recommendation is to create a meta, then pull out the smallest of the three subsystem sets from this MR, and do what @longwave is suggesting for it.

I asked for the scope of this to be split up back in #48.

22 followers is also actually a lot.

Thanks @tedbow for so much work getting this handy feature into core!

Note to maintainers: Ted is still a maintainer for Update Status and Layout Builder, and may also be a maintainer of a couple core-tracked modules we are desperately trying to get to beta. So he should still have perms on the channel, the core node, etc. after this issue.

xjm → made their first commit to this issue’s fork.

@Jelle_S has been contributing in this role for over a decade now! Thank you so much for your work hauling core into the 21st century with responsive images.

xjm → made their first commit to this issue’s fork.

Random fails are critical. 🙂

Thanks @mpdonadio for all your work on these tricksy, tricksy subsystems! (Not to mention menus and routing and so forth back in the day; those were some criticals eh?) It's been great collaborating with you. You're also welcome to rejoin the maintainer team in the future if you're interested and your availability permits. If not, best wishes for all your other endeavors.

Committed to 11.x, 11.2.x, 10.6.x, and 10.5.x. Since this is both of @mpdonadio's subsytems, I'm also updating the Google group, the core project node, and the Slack channel.

Thanks!

Thanks @rainbreaw! Your expertise has been an awesome resource.

The MR did not apply, but this is the sort of issue that committers can just fix directly, so I made the change manually and committed that. Committed to 11.x, 11.2.x, 10.6.x, and 10.5.x. Since this was Rain's single MAINTAINERS.txt entry, I'm also updating the core node permissions, the Google group, and the maintainer Slack channel.

Note that I did not credit @ankusht1515 on this issue. (We are going to limit it to a couple credits for MR creation by itself, because these issues should have been created as postponed, oops). They are credited on another of these issues though for help creating all the MRs. 🙂

Thanks everyone!

Thanks David! Committed to 11.x, 11.2.x, 10.6.x, and 10.5.x. Also updating the core node maintainer permissions, the Google group, and the Slack folder.

I am crediting @ankusht1515 here for help creating these merge requests. Congratulations on your first core issue credit!

Agreed @fago; any issue that implements a fix for this can be treated as a critical UX bugfix (even if it requires a feature or API addition).

@andrewmacpherson, thank you so much for all your dedicated work on core's accessibility! You made a huge difference for core in helping us to think critically about accessibility for so many major core features. I still think of your input whenever I remind myself or others that we need to put accessibility first.

Also, thank you for teaching me a marvelous new cuss:

Sod this for a box of frogs

Committed to 11.x, 11.2.x, 10.6.x, and 10.5.x. I will also update the core node, the Google group, and the maintainer channel since this was @andrewmacpherson's single maintainer role.

MWDS ❤️

What an excellent maintainership application! Thank you everyone for you feedback.

Welcome to the team @dcam! I've granted you permissions on the core release node that will allow you to do things like administer core issue credit, manage some aspects of merge requests that are not your own, etc. The first change you will see will be that the assigned field is suddenly massive.

I will also add you to our core maintainers channel.

Committed to 11.x, 11.2.x, 10.6.x, and 10.5.x. Thanks!

Thanks both!

Clarify the point about bug fixes. Many bug fixes are disruptive and therefore minor only, require CRs, etc.

Adjusting credit. Thanks!

When this is committed, remember to tag the issue for the release notes of the correct 11.2.x patch release (which is 11.2.3 as of now).

A few things to improve. Thanks!

NW for a number of small fixes.

Looks like the pipeline got stuck, which apparently happens when a committer rebases a branch, commits suggestions, etc. without having access to the fork. I tried to fix it by getting access to the fork and then re-running the pipeline.