πŸ‡ΊπŸ‡ΈUnited States @tim.plunkett

Philadelphia
Account created on 14 February 2008, over 16 years ago
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

@phenaproxima and I discussed this over the past few days, this is ready now.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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)

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

@catch that would explain it!

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

`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.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Looks good, thanks

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

tim.plunkett β†’ changed the visibility of the branch 2293803-replace-confirm-password to hidden.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

@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 β†’

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

tim.plunkett β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
+++ 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.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Removing resolved "needs" tags

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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!)

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
  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?

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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?

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

πŸ› 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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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()

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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 β†’

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

Production build 0.69.0 2024