🇺🇸United States @tim.plunkett

tim.plunkett → created an issue.

Thanks!

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

from @longwave in Slack:

MRs against 11.x are ok and will be preferred once 11.0.0-beta1 is out and 11.1-only changes go into 11.x (yes this is confusing, can't wait until we can use main)

tim.plunkett → created an issue.

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

@catch that would explain it!

@alexpott I would have thought the `->reveal()` calls would have helped, but fair enough

I'm not able to run the tests-only job for some reason. But when I do locally, I get

Failed asserting that Double\ConfigurableInterface\P1 Object (...) is not an instance of interface "Drupal\Component\Plugin\ConfigurableInterface".

Which sounds like a Prophecy mixup, not an actual working assertion.

@alexpott am I overthinking it? The change looks good

tim.plunkett → created an issue.

tim.plunkett → created an issue.

I will try to look between now and the end of DrupalCon. For now, I'd consider this NW for finding a better way

lauriii → credited tim.plunkett → .

`needs design` literally means that it needs a designer. Which is different than usability.

I don't know that a UI designer ever looked at this, as opposed to solely front-end and back-end developers.

ckrina → credited tim.plunkett → .

Say you carefully configure your view display to have the exact order and field formatters you want. You save, everything is great. Then you wonder what might happen if you turn LB on. You try it, decide against it, and then disable it.

In HEAD, all is well. you're back to your perfectly configured display.

After this MR, everything is gone. You're back to square one.

Left some suggestions for nits, but also raised a few points that need to be addressed.

What's the stack trace of the exception? Whatever it is, it's happening before layout_builder_entity_type_alter() has run, which shouldn't be possible.

I don't remember which hooks we had or if they were "standardized" yet, but 6 years ago when I opened this, I said:

Allow the derivers themselves to affect the list of cache tags used.
This will remove the need for external code (like a hook) to clear the cache.

So I don't know that this has any practical application anymore if we're okay with needing hooks forever

Looks good, thanks

amateescu → credited tim.plunkett → .

I've asked @omkar.podey to mark resolved threads with ✅ so @srishtiiee can close them

Test

Merged! Thanks

Saving credit

tim.plunkett → changed the visibility of the branch 2293803-replace-confirm-password to hidden.

Answered the question, fixed the nit, and performed my own review. Thanks @kunal.sachdev!

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

@pyaephyohein that's 🐛 PHP notice "Undefined index: region" on layout overrides when using the Field Layout module Needs work , different module.

Thanks @clayfreeman and @godotislate for keeping this one going.

Running the 'tests-only' pipeline shows a fail as expected. Keeping NW for the IS update

+1 to RTBC, the Drupal.ajax vs ajax discussion in the MR can be resolved

@effulgentsia and I discussed this just now.
We're fine with committing this, but think that this needs it's own CR pointing out that if you choose to use closures in your config targets, then you will lose the ability to have any sufficiently complex AJAX handling on that form (anything that approaches the complexity of a multi-step form).

This is mitigatable from two directions:

1. Simplify your config forms to not be multi-step
2. Rewrite your closure as a static method

I don't know of any forms that would need the advice of #1.
And taking the example of SiteInformationForm, there is no reason that fromConfig: fn($value) => $value ?: ini_get('sendmail_from'), couldn't be a static method on the form...

Consider this a +1 from the Form/AJAX system maintainers, and if you want to relive some of the digging I did on this, enjoy reading all the child issues of #635552: [meta issue] Major Form API/Field API problems →

+1 for fixing the "Displays Settings" typo. The new layout is great!

As this is targeting 10.3 and won't be backported to 10.2, here's a patch for 10.2

tim.plunkett → created an issue.

+++ b/token.tokens.inc
@@ -569,7 +569,7 @@ function token_tokens($type, array $tokens, array $data, array $options, Bubblea
+    if (($url_tokens = \Drupal::token()->findWithPrefix($tokens, 'url')) && $node->id() !== NULL) {

Could also use !empty($node->in_preview). That pre-dates LB (think original node preview) but is also set when LB generates the sample entity.

#284 that is being worked on in the one child issue: 📌 Restrict access to empty top level administration pages for overview controller Fixed

Removing resolved "needs" tags

"Bumping" is unhelpful, now the issue is further down the RTBC queue

#12-14, this is a meta/plan issue, please open a dedicated issue to propose that change.

The changes in !5094 seem reasonable, but are unrelated to this issue. Nothing in that changeset has anything to do with modals. I'd ask that you create a new issue for your refactoring so we can focus on one MR to accomplish implementing modals.

@alexpott pushed back on explicit test coverage for double-saving, but the last commit fixes things nicely enough

One piece of feedback. Also there are 17 other unresolved threads that *were* resolved. @godotislate as the MR author, you can mark those as resolved.

smustgrave → credited tim.plunkett → .

Attempted a rebase now that combined forms is in. I did not fix the bugs introduced in the last commit

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

Reviewed all the changes since the end of September, and I'm +1 to this being RTBC.
I especially like the Subform changes, I think this marks the first core usage outside of plugins.

Thanks to everyone who worked on this, but especially @srishtiiee who carried it for so long, and to @lauriii and @alexpott for getting it to the end (I hope it's the end!)

I can see why it seems that way, but we found these discrepancies when dealing with *empty* admin pages, and to do so you'd need to remove a bunch of the admin items

The prettier reformatting was so jarring I didn't even notice that it was existing code, lol sorry for being distracted.

The comment change and added () make that better, thanks.

1. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -327,6 +333,16 @@ public static function preRenderAjaxForm($element) {
   +      $text_types = ['password', 'textfield', 'number', 'tel', 'textarea'];
   ...
   +        if ($element['#ajax']['event'] === 'blur' || $element['#ajax']['event'] === 'change' && in_array($element['#type'], $text_types)) {
   

   These 5 types all have their #ajax][event type set to blur unless it's manually set to something else... Not sure if that's what this is protecting against or not.

   Additionally, the mixing of || and && without parentheses makes this extremely hard to parse.

2. +++ b/core/misc/ajax.js
   @@ -1083,19 +1094,30 @@
   +                for (
   +                  let n = elementParents.length - 1;
   +                  !target && n >= 0;
   +                  n--
   +                ) {
   

   Is this how JS wants us to format for loops?

   Also, any reason to not use the more standard i instead of n for the counter?

Work can continue in the MR, but for the short term here is a snapshot of the changes backported to 10.1.x

The STR include enabling Pathauto, but as @berdir points out in #180, this should be reproducible in vanilla core.
Can someone update the IS to explain how to reproduce this without Pathauto?

I think this should be postponed on 🐛 Page title should contextualize the local navigation Needs work

Adding some related issues

nicxvan → credited tim.plunkett → .

According to manually testing with git bisect, https://git.drupalcode.org/project/drupal/-/merge_requests/4366/diffs?co... is the offending commit.

Reverting that fixes things, but I'm not sure why @omkar.podey added that.

And as @hooroomoo points out, we're definitely missing test coverage

🐛 Field [storage] config have incomplete settings until they are saved Fixed is in! Rebased the branch, but there are few unresolved threads as well as a few @todos pointing to the blocker

This is hitting #3211992: TestSiteApplicationTest::testInstallWithNonExistingFile() fails when another test creates database tables during the test run → which was supposedly fixed but was not

No, there's nothing wrong with overview. It's just confusing. Hence the added docs in the MR. We're just switching it from 2 usages to 1 usage.

But this might not even be needed, postponing on 🐛 Restrict access to empty top level administration pages Fixed to see how that shakes out

After spending the afternoon digging into 📌 Restrict access to empty top level administration pages for overview controller Fixed , I think the last commit I pushed belongs in this issue and negates the need for the follow-up.

Both controllers use \Drupal\system\SystemManager::getAdminBlock() to display links, but differently:

SystemController::overview() iterates over the links 1 level down, and uses getAdminBlock() to retrieve the links 2 levels down. It displays all of the blocks in a grid.

SystemController::systemAdminMenuBlockPage() uses getAdminBlock() to retrieve the links 1 level down. Since there is only one "block" retrieved, it displays those directly, no grid.

I took screenshots of /admin/config and /admin/content with both of the possible controllers.
Note that I had to uninstall the node module first, thanks to the route subscriber mentioned in #3. This makes all of this a lot more theoretical, since keeping /admin/content in the admin menu while the node module is uninstalled seems like a dramatic edgecase.

/admin/config with SystemController::overview()

/admin/config with SystemController::systemAdminMenuBlockPage()

/admin/content with SystemController::overview()

/admin/content with SystemController::systemAdminMenuBlockPage()

Interesting that node has \Drupal\node\Routing\RouteSubscriber, which swaps out the controller and requirements.

\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage() was added in Drupal 5(!) as system_admin_menu_block_page()

\Drupal\system\Controller\SystemController::overview() was added in Drupal 7 as system_admin_config_page(), and was originally added only to represent /admin/config. It was reused for /admin/content in #2085571: admin/content should not depend on node.module →

This is fine from a Views perspective. Sorry for the 100s of test views, seeing all those brings back memories :D

@nginex I rebased the MR (which anyone can do) and @joachim changed the target (which I asked for in Drupal slack)

@znerol would be great to get your thoughts on the PoC that Yash linked to in #117

I'm not sure if we'll get away with not having a CR but it would be nice to have an updated IS 😇

nicxvan → credited tim.plunkett → .

That's some mid-2014 era luck 🫨

@yash.rode helped me get set up today and I'm also having the same issue where despite calling session_start(), the superglobal $_SESSION doesn't exist.

It definitely exists when putting a breakpoint in a random place like the node form, and it's full of Symfony variables...

Not sure what is missing here.