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

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

Merge Requests

Recent comments

πŸ‡ΊπŸ‡Έ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 RTBC

πŸ‡ΊπŸ‡Έ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 RTBC , 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

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

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

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

That's some mid-2014 era luck 🫨

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

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

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

Thanks @Wim Leers for addressing my (and @effulgentsia's) feedback! +1 to RTBC

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

This is an API addition to an unreleased feature, I agree it doesn't need a CR

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

Just struggled with this in another issue last week, let's see if we can come up with something

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

Assigning to myself so I don't lose it

lol, whoops

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

Should have been unpostponed 5 years ago, whoops

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
  1. +++ b/core/lib/Drupal/Core/Form/ValidatableConfigFormBase.php
    @@ -0,0 +1,98 @@
    +    protected TypedConfigManagerInterface $typedConfigManager
    ...
    +      $container->get('config.typed')
    

    Nit: trailing comma, please

  2. +++ b/core/lib/Drupal/Core/Form/ValidatableConfigFormBase.php
    @@ -0,0 +1,98 @@
    +    foreach ($this->getEditableConfigNames() as $config_name) {
    +      $config = static::mapFormValuesToConfig($form_state, $this->config($config_name));
    

    Consider adding some assertion here to ensure the config hasn't already been saved (it shouldn't be!)

  3. +++ b/core/lib/Drupal/Core/Form/ValidatableConfigFormBase.php
    @@ -0,0 +1,98 @@
    +    foreach ($this->getEditableConfigNames() as $config_name) {
    +      $config = static::mapFormValuesToConfig($form_state, $this->config($config_name));
    
    +++ b/core/modules/update/src/UpdateSettingsForm.php
    @@ -116,34 +84,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +  protected static function mapFormValuesToConfig(FormStateInterface $form_state, Config $config): Config {
    +    $config
    +      ->set('check.disabled_extensions', $form_state->getValue('update_check_disabled'))
    

    This is called once per editable config, and while this implementation only has 1 available, I think as the first and only example in core this should have a switch/match/if on $config->getName() to demonstrate how that would need to be handled

  4. +++ b/core/modules/update/src/UpdateSettingsForm.php
    @@ -2,50 +2,18 @@
    +class UpdateSettingsForm extends ValidatableConfigFormBase implements ContainerInjectionInterface {
    

    The implements ContainerInjectionInterface was never needed, but now it's confusing without the create() override

  5. +++ b/core/modules/update/src/UpdateSettingsForm.php
    @@ -116,34 +84,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    if ($property_path === 'notification.emails') {
    +      return 'update_notify_emails';
    

    I'm guessing it's because it's the only one we know has any config validation, but why is this the only one mapped? The other 3 values have mismatches too. I think as the example form being converted, this should establish a pattern for doing this for all 4 properties (whether it's switch/match/if)

I was about to write a comment about the mutable vs returned Config but I see @effulgentsia brought it up just now.

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

+1. Copying credit from the other issue.

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

11.x is a placeholder. All changes are made in that branch first and then backported to 10.1 or 10.2 or whatever.

Also there's a merge request now, the patch doesn't need to be used.
If you need one, you can append .diff to the merge request URL, like https://git.drupalcode.org/project/drupal/-/merge_requests/4390.diff

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

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

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

Note that in addition to clearing Drupal caches I also ran sessionStorage.clear();localStorage.clear(); in the browser console while testing this just to be sure. Works for me. Pushed a small clean-up

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

@srishtiiee points out that these 5 are in the General category. My suggestion was to switch from a preprocess hook to hook_library_info_alter implementations, with each adding their own icons to a General category library.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
$ grep -nrA3 hook_preprocess_form_element__new_storage_type * | grep icon

core/modules/datetime_range/datetime_range.module-35-  $variables['#attached']['library'][] = 'datetime_range/drupal.datetime_range-icon';
core/modules/comment/comment.module-791-  $variables['#attached']['library'][] = 'comment/drupal.comment-icon';
core/modules/telephone/telephone.module-42-  $variables['#attached']['library'][] = 'telephone/drupal.telephone-icon';
core/modules/link/link.module-73-  $variables['#attached']['library'][] = 'link/drupal.link-icon';
core/modules/media/media.module-540-  $variables['#attached']['library'][] = 'media/drupal.media-icon';

There are still 5 modules using the preprocess hook to add in icon libraries.

Production build https://api.contrib.social 0.61.6-2-g546bc20