🇺🇸United States @xjm

Committed to 11.x, and cherry-picked to 11.2.x as a patch-safe docs bugfix. Thanks @mstrelan!

This issue is currently doing a lot more than correcting outdated references to https://www.drupal.org/docs/8/theming. 😀 Comments on MR. Thanks everyone!

I am trying to understand how this has been wrong for 11 years.

Is there a way we can validate the whole file?

I checked and both handbook pages have been updated, so I think this can be marked fixed now!

However, maybe we technically need this change to be made in Drupal 12 (not 11) since technically we shouldn't change platform requirements in a minor?

We should add a snippet to the 11.3.0 release notes either way.

Now testDanglingReferencesInAnEntityReferenceFieldFromIssue2968972() is a method name, y'all.

Yay actually following documented typing for strict type enablement potential. Committed to 11.x and cherry-picked to 11.2.x as a patch-safe cleanup to individual tests. Thanks everyone!

Committed to 11.x. I considered backporting it to 11.2.x without the rule with some fancy git tap-dancing, but there are also at least two merge conflicts with the fixed comments, so since this is just basically silly typos I think it's safe to leave this one as 11.x only.

Thanks everyone!

@longwave I'm not sure they've officially agreed on that yet; one of the maintainers is proposing that and hopes to get agreement on it but it might not be 100% confirmed yet.

Tested it by committing 📌 Forbid '@encode' annotation RTBC .

Thanks @joaopauloscho and @nicxvan!

Thanks @danielveza for reporting this. I do agree that using a machine name that references a real existing module and block is misleading.

I disagree though with the proposal of fixing it by using random machine names. We know from long experience that random values in tests are almost always a bad idea. They don't substantively add to test coverage, they make tests harder to read, and they substantively increase the number of random test failures. For example, see my comment on the merge request.

I would be better to come up with fixed test strings for each test block that describe the purpose of that test block.

Thanks!

Nice, thanks @quietone!

I went to look for this in the RTBC queue and discovered 📌 Set memory limit for `composer phpcs` command Needs review instead. 😀 Even if that goes in, I still think this change is worth making. (Haven't had a chance to test yet though; will do so when I can unless someone else beats me to it.)

Committed to 11.x, and cherry-picked to 11.2.x as a patch-safe test bug fix. Thanks everyone, and thanks for participating in the DrupalCon Vienna contribution sprint!

Created the followup: 📌 PluginTestBase is not actually a "base class for plugin API unit tests" Active

xjm → created an issue.

I think @ranzinator2000 is the d.o username for @manuel-ranzmeir and have updated the contribution record accordingly. Please let me know if this is not correct!

The diff is not applying, so I attempted a rebase through the UI.

I checked the base class and confirmed that it does indeed have secret dependencies on both Node and User. I also confirmed that the three tests updated in the patch are its only descendents. Nice find!

@berdir re: #6, we actually raised some of the same concerns about the architectural entanglement with comment in #3336276-4: [Policy] Deprecate History module → . When it was discussed again 18-some months later at DrupalCon Barcelona there was consensus among the Core Leadership Team to sign off on remove it, but that doesn't mean we're requiring that it be removed before Drupal 12 or anything. So we did take it into consideration, but I can also definitely see it happening in D13 instead of D12.

I was talking about this with @andypost in person at DrupalCon Vienna while reading @berdir's comment.

Discussed with @alexpott. Having this dependency added separately from ✨ Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments Active is a good separation of concerns; however, we don't want to end up in a situation where this dependency is added to a minor release without the functionality it unlocks. Since we're pretty close to alpha1 of 11.3 and we probably don't want to make a change as dramatic as ✨ Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments Active in that release since it's so soon and it would not give us enough time for review and testing, we agreed to postpone this issue on 11.3.x being branched. Then potentially commit it to 11.x after that. I'm adding this issue to 🌱 [meta] Release preparation steps for 11.3.0 and 10.6.0 Postponed to keep track of it.

Thanks!

I got to overhear the reasoning for ✨ Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments Active earlier today and everything here convinces me this is worth the tradeoff of the additional dependency. Thanks @kingdutch!

xjm → changed the visibility of the branch 3549350-remove-useless-code to hidden.

Whenever we remove dead code, it's always a good idea to research the history of the issue to see how it became a dead code in the first place, to make sure we're not accidentally removing the last remnant of an intentional behavior or the last evidence of a bug.

In the case of this issue, I wanted to make sure that removing this condition was in fact a pure cleanup rather than some edgecase that still needed support, so I asked for signoff from either an Ajax subsystem maintainer or a frontend framework manager. @realityloop checked with @tim.plunkett, who researched the history of this issue all the way back to 2013. Tim also reviewed the code side by side with @justafish who is one of the frontend framework managers.

I've also reviewed the code in context myself to confirm that it is indeed dead code since there is nothing in the caller tree to set #typeto ajax. (I'm not totally sure, but I think this may date back to a very old bug with Ajax responses being created within Ajax responses, which is why it got carried forward from very legacy code into modern Ajax response rendering.)

In any case, based on the above signoffs, I'm comfortable committing this to 11.x HEAD.

Thanks everyone for your work on this issue!

I think the minimal scope here of removing the broken link is still a good novice task. We can simply remove the part of the sentence after the comma. The followup issue can handle adding additional content as needed from the historical reference.

I think this is probably not novice since it is not clear what the path forward is. Thanks @arunsahijpal for opening the followup!

We should update the IS here based on the discussion in the original issue.

Discussed this issue with @igorgoncalves. I think that in context the "see below" refers only to the term "key column specifiers", which are indeed documented in detail below. Therefore, as a Drupal core release manager, I am marking this wontfix. Thanks for taking the time to review this and make suggestions!

Rerolls aren't a thing anymore.

xjm → credited xjm → .

I updated 🌱 [12.x] [meta] Set Drupal 12 platform and browser requirements at least six months before the release Active to explicitly link the new policy doc that @quietone created. I think that should be sufficient as it will be copied forward for the D13 meta etc.

It is also our policy that the platform requirements are set at least 6 months before the major release date of the major and https://www.drupal.org/about/core/policies/core-change-policies/set-plat... → could be linked wherever that is in the docs (I forget exactly at the moment).

There is one issue that I noticed. It says:

The minimum PHP requirement is set to the highest stable PHP version available at the time beta1 of the next major version is released.

However, that potentially conflicts with the existing requirement that the platform requirements be set at least six months before release. It wouldn't happen with the current release windows that we and PHP have, but suppose that PHP changed their schedule to release a new stable version in February. The old policy would tell us that we couldn't both increase our PHP version requirement and have a June release, but the new policy would indicate that we must increase our PHP version requirement.

It's a hypothetical scenario and I think we would sort it with common sense, but we might want to tweak the language slightly to account for the "six months before" bit, or even add it to the top of the new page since it's existing policy about platform requirements.

benjifisher → credited 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.