🇺🇸United States @neclimdul

longwave → credited neclimdul → .

Fix in the MR.

One bonus benefit of checking the display_id like this is we can completely delay loading the view executable until the lazy builder, further optimizing the lazy path and only loading it once.

neclimdul → created an issue.

Ran into one of the instances of this today and went "what is that word?!" and now I find myself here. So I guess I support converting to just "cast" I guess :-D

I was confused why we hadn't changed default.settings.php and the answer is that its not currently there. This is one of those power user settings we don't document and only set if you're running into an edge case.

I suspect that will continue and changing that would be its own issue but I understand how its tied up in this. I think we can mitigate this by linking to the documentation in the changelog and maybe release notes so in the unlikely case this does push someone into the edge case they know how to resolve it.

Also conflicted with 🐛 Performance Degraded after update to twig 3.14.2 Active

merge request made and passing so back to NR.

The performance regression changes where a bit disruptive. They make this change kinda BC breaking because now the settings are directly exposed in the less secure expression format. Which is really frustrating and I don't really know how to resolve it.

broken by 📌 Remove misc usage of whitelist in tests and comments Fixed . reroll soon.

Technically I think it was added in 8.3 or 8.4 so its pretty recent even in the SB8 releases. We're using the --no-open workaround.

Fixed

working on logging and tests to make this a better long terms solution.

larowlan → credited neclimdul → .

Can't really provide steps to reproduce, had an environment with some broken out of sync config in the db that included the stable theme. Applied a similar patch to the merge request and it worked to get us out of a jam to where we could run drush and fix the site and get the config in sync.

This also fixed the tabledrag in 10.3 for us as well.

It seems to revert the previous issue but I thought the centering was an improvement so that's fine with me.

quietone → credited neclimdul → .

Guess eslint is EOLing this weekend(tomorrow)?
https://eslint.org/blog/2024/09/eslint-v8-eol-version-support/

plugin-import rushed an update out this week.
No update on airbnb and comments asking for an update have just been marked as spam...

benjifisher → credited neclimdul → .

neclimdul → created an issue.

Yeah, digging into it, its a pretty hard problem and current Drupal asset and script attachment is working against this module at every step so I see how the module ended up where it is.

As time allows I'm playing with some solutions. drupalSettings with attached javascript, attachinline.module, etc.

Likely something with drupalSettings is the only solution to get something at the top and not cached because Drupal has special allowances to make the settings play well with cached. But it also turns out it is very difficult to target specific orders to place this before the tag manager script and after the settings script.

📌 Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering Needs work aims to improve this but it isn't committed yet so it is probably a ways out from being adoptable as a real solultion for this module.

I'm leaning toward a best effort with drupalSettings and a javascript behavior with documentation on how to read from drupalSettings in tag manager using variables as a fallback for sites that run into trouble. This also improves CSP stuff by getting rid of unsafe inline scripts so it should be a win/win if I can get it working.

Extended this fix to the access control class which has the same logic and ported the patch to a merge request.

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

I think technically this being at the bottom works but being in the footer does make using the datalayer much more complicated. The values aren't available for early hooks meaning if you're trying to pass them to something like the page view event they aren't available.

Additionally, since they only push the values on, not triggering any events, there isn't a way to trigger pushing any values into a tag.

It seems like it would cause fewer problems for developers if it was in the header somewhere. even better if it could go _before_ tag manager.

Doing a reroll I tried to understand the subquery changes Charly was trying to do. They weren't working for me and the previous code is pretty obtuses and admits its fixing confusing edge cases so I made a branch without it since I believe it was working for us as is. Split his changes into a separate branch if anyone wants to view and compare.

Sorry, about the branch noise. I forgot to request access and before you do the branch names get mangled and I didn't notice until after I pushed them.

neclimdul → changed the visibility of the branch 2379423-representative-node-views to hidden.

benjifisher → credited neclimdul → .

Oh wow, this is an old one. Thanks!

I realized this only works if you've already updated to 10.3. If this was released and you applied it to a 10.2 site prior to upgrading you'd be in the same position. Not sure how to address that though...

Looks like this worked for us as well.

Forgot about this. Thanks, committed.

I think this is documented and reviewed to death at this point. Its actually needs work because it needs tests as the tag suggests.

I keep forgetting Drupal even provides commands. They aren't quite at the point the can replace any drush workflows for me though I guess.

Maybe we should start moving this in front of people more. The Drupal console isn't active these days so since it won't conflict could we start pushing core/scripts/drupal into vendor/bin/

Then maybe we could start looking at using a discovery mechanism of some sort for auto discovering commands instead of hard coding them.

Then we could encourage people could start tinkering with this, testing real world use cases, without hacking on core as directly?

longwave → credited neclimdul → .

Oof. So the alias validation is tied to the site language in the admin interface? That sounds complicated.

Checking GL, it does say "Merge conflicts must be resolved." so think this needs another rebase.

No worries! Thanks!

That's... interesting. It sounds like we're on the right track though so I'll take another pass at improving this.

Clearly relying on that live region isn't working out well. It looks like it doesn't announce anything if the removal is at the end and I suspect it doesn't behave all that well when the change is at the beginning and there are multiple items. That might be connected to the "oblivious" example as well, live regions and lists have been a source of frustration and I think nvda explicitly documents they might not work.

I'll poke around this time with direct announcements where we can control the behavior better. That will make things easier anyways since I can use devel_a11y to review things quicker.

Thanks, yeah adding that link would be a simple way someone could see it.

Anyone could update issue summaries.

Some initial testing and it looks good to me. As expected, fixes some bugs I was seeing as well.

Would be great if someone could take a look at this and see if its on the right track before I spend too much time trying to polish it.

I was on alpha but it seems this is fixed in the latest dev release. Should have checked that first :-D

Branches seem a bit confusing. Switching to the branch with code.

neclimdul → created an issue.

Not the decision maker but just going to re-iterate, I think this is a bad idea. The hacks I see for uninstalled modules seem like just the tip of the iceberg. What about removed modules. removed libraries. uninstalled themed. profiles.... it seems like a loosing battle for little benefit.

The new config format is kind of confusing but it looks like it might be useful.

Playing around with the compat layer and eslint-config-drupal I found a couple things to keep an eye one that are kinda blocking.

1. airbnb config isn't ready. Think this is the right issue tracking it. https://github.com/airbnb/javascript/issues/2804
2. plugin-import isn't ready https://github.com/import-js/eslint-plugin-import/issues/2948
3. valid-jsdoc was removed which will require some changes. https://github.com/eslint/eslint/issues/15820

Seems like things are still catching up in the community so don't know if the other plugins need changes to support v9 but that could be a problem.

@xjm with the exception of my concern the link generated from the code in the changelog might store csrf links in caches, yes that addresses it.

I see how my comment was misunderstood. It was technically in the sites themes but we have several form designs so a new form has to be assigned to a specific look or it just ends up "unthemed". On our site, unthemed was quite ugly.

More my point was more that any form isn't desirable and a fair few years of building Drupal sites has made me lazy about dropping a menu link in a template.

While we're discussing it, I haven't don't much testing but I think I echo some concerns from 14 years ago about caching. I haven't tested yet but is the CSRF token going to end up cached in templates using the code in the change log? Should core provide a lazy builder?

left rtbc because this isn't a change this merge makes just something it highlights so could be a follow up.

I think I understand the reason of the change and it makes sense. Previously you'd have to return true from applies if you _might_ apply and then builder would have to do the actual check and apply the cache-ability. Yeah, that's a pretty big gotcha when building these builders.

Looking at the merge, there's a lot of duplicate adding of the route to the cache-ability. I guess maybe this is a dumb question and probably well understood in the building of BreadcrumbManager but isn't varying by route kinda a baseline implied functionality for breadcrumb builders? That's why we pass the route_manager in right? Should the manager just include that by default and this change would only be used for more exotic cases?

If it helps the discussion, and example where it doesn't cache by route in core is PathBasedBreadcrumbBuilder. I found that's broken on our site and actually decorate the core version with one that adds the route to the cache-ability of the path breadcrumb builder.

Don't see it in the gdoc but 🐛 User logout is vulnerable to CSRF Fixed might be a disruptive change for sites. Had some hard coded /user/logout links in some templates which lead to a pretty ugly unthemed confirmation form.

Notable change is the adding of the session to the ajax response as I was getting "Session has not been set." for both anon+auth users as I have cookies for all users in use.

Failed to load resource: the server responded with a status of 400 (Bad Request): /facets-block-ajax?_wrapper_format=drupal_ajax:1
Uncaught Drupal.AjaxError: ajax.js?v=10.1.6:1194
StatusText: Bad Request
ResponseText: {"message":"Session has not been set."}

Adding it to the request not the response but this is a good find. I ran into this testing 10.3 and my ajax facet blocks where suddenly broken. The block in FacetBlockAjaxController that does this solved the problem for us.

I'm not sure I understand why we're not using standard methods for attaching things to the footer.

Not sure of a common real case but I was able recreate this just now like this.

1. Install Drupal site.
2. Ensure registration is allowed.
3. Log out and visit the registration url with a complex url fragment. e.g. https://d10.lndo.site/en/user/register/#badfragment/broken

xjm → credited neclimdul → .

Took a stab at laying down some ground work for the suggestions. This actually is pretty cool because a lot of the hardcoded things that made it impossible to really theme or extend the password strength region are tied to the strength indicator so focusing on the suggestions allows it to be more extensible which can sort of be seen in the latest updates.

I don't claim this is a "good" implementation but it hopefully keeps things moving forward.

Nice!

Yeah, probably not measurable from a wall clock perspective with current PHP performance hence the title. However, it removes one assignment, one function call to implode, and the admittedly trivial string concats implode is doing from every twig_render. This means on a stock Drupal install it saves 54 function calls and assignments on an uncached request to the frontpage so its at least measurable from that metric. Since most sites are probably more complex then that and as you said its actually a bit cleaner it seemed worth while.

Makes sense. I remember browser instrumentation generally not working so I've never run it though so I've got some questions for people that do use it:

1. Are there possibly additional problems with caching with how manual adds adds code? Specifically, is the "header" being added unique to the request? I see the settings tag but should this be a lazy callback or have additional tags to avoid page/render caches?
2. In the post update, should it take the Drupal version into account and migrate to manual instead of migrating to a auto that behaves like manual with a not immediately obvious warning? Or maybe it should warn the user during the update? Unsure.

reroll in MR

Quick reroll on MR

Its not a "problem" per say. It seems the way the code gets run now the output isn't being captured but is instead ending up being output.

From memory, { "command":"buildAll"} is the response from solr saying "I recieved the buildAll command from you and ran it."

I actually agree and disable phpinfo in production systems entirely. I think in previous discussions some people wanted phpinfo to be available in production so people could check modules and other php configuration which is what lead to the current approach and why I suggested the more target hardening. But also not trying to blow the scope, just wanted to be mindful of an impact.

Core has some protections in place to avoid these sorts of sites leaking critical information after https://www.drupal.org/sa-core-2023-004 →

But if we're making this a core feature we'll be encouraging people to do even more, a good thing imho. But since this means it expands the places and levels of experience using the feature, it does have me wondering if when adding the core feature, we should also harden it to it warns people if they have their system configured in a way that could be unsafe. Maybe a requirement check that detects environment variables would be available through core's phpinfo and this DB auto-configuration is being used it throws a warning like we do with settings directory permissions or allow_updates.

created

Test should be fixed.

I think the concerns might be a bit overstated. Actual problems will trigger real exceptions that get logged. This just avoids warnings that shouldn't be triggered. If I remember correctly they're warnings triggered by the underlying libraries so they don't actually follow PHP's documentation for the method.

got some tests in place that identify some of the pending issues. not passing because we've got some hard problems to solves still.

re: 📌 Simple fixes for words with prefix of 'de' or 're' RTBC

dequeue is the correct technical term for removing an item from a queue. I'm not sure if unqueue is actually a word but what I can find it has a different meaning.

reference:
- https://en.wikipedia.org/wiki/Queue_(abstract_data_type)
- https://computersciencewiki.org/index.php/Queue
- https://www.programiz.com/dsa/queue
- etc

These changes where just accidentally dropped from a re-roll. Patch to re-introduce them.

neclimdul → created an issue.

Got a test that's failing against core:

faddb2be4808:/app/core$ yarn test:nightwatch --test tests/Drupal/Nightwatch/Tests/a11yTestPager.js 

[Tests/A11y Test Pager] Test Suite
────────────────────────────────────────────────────────────────────
ℹ Connected to chromedriver on port 9515 (93ms).
  Using: chrome (106.0.5249.103) on LINUX.

  ℹ Loaded url http://d10.lndo.site in 259ms
  ℹ Loaded url http://d10.lndo.site/user/reset/1/1714417934/bnXISZHcCy0Vaj7yqQkhlbKvsb2pb2R8YKembDx4hMY/login
 in 384ms
  ℹ Loaded url http://d10.lndo.site/admin/modules in 381ms
  ✔ Element <form.system-modules [name="modules[pager_test][enable]"]> was visible after 528 milliseconds.
  ✔ Element <#system-modules-confirm-form> was present after 514 milliseconds.
  ✔ Element <form.system-modules [name="modules[pager_test][enable]"]:disabled> was present after 8 milliseconds.
  ℹ Loaded url http://d10.lndo.site/user/logout/confirm in 168ms

  Running pager ellipsis is accessible:
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
  ℹ Loaded url http://d10.lndo.site/pager-test/ellipsis in 81ms
  ✔ Testing if element <.pager__item--ellipsis> is visible (20ms)
  ✔ Passed [ok]: aXe rule: aria-allowed-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-allowed-role (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-conditional-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-deprecated-role (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-prohibited-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-required-attr (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-roles (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-valid-attr-value (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-valid-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: color-contrast (2 elements checked)
  ✔ Passed [ok]: aXe rule: duplicate-id-aria (1 elements checked)
  ✔ Passed [ok]: aXe rule: empty-heading (1 elements checked)
  ✔ Passed [ok]: aXe rule: landmark-unique (1 elements checked)
  ✔ Passed [ok]: aXe rule: link-name (3 elements checked)
  ✔ Passed [ok]: aXe rule: listitem (4 elements checked)
  ✔ Passed [ok]: aXe rule: presentation-role-conflict (1 elements checked)
  ✖ NightwatchAssertError
   aXe rule: list - <ul> and <ol> must only directly contain <li>, <script> or <template> elements
        In element: .pager__items

  FAILED: 1 assertions failed and  17 passed (385ms)

────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

  ️TEST FAILURE (7.154s):  
   - 1 assertions failed; 20 passed

   ✖ 1) Tests/a11yTestPager

   – pager ellipsis is accessible (385ms)

   → ✖ NightwatchAssertError
   aXe rule: list - <ul> and <ol> must only directly contain <li>, <script> or <template> elements
        In element: .pager__items

 Wrote HTML report file to: /app/core/reports/nightwatch/nightwatch-html-report/index.html

Rebased and pushed test to the MR.

Looked at this some. The asset rendering rendering pipeline is a mess and fights this at every step so I'm not even sure is going to work without updates to core. Some many things change in the sort depending on if its optimized or not. there's all the grouping. And each component relies on the logic of the other steps and assumes only internal and external files are possible.

Also found this that's probably related in some way. 📌 Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering Needs work

Going to keep hacking on this but its pretty complicated and at the very least it can't really happen as late as the renderers.

You can't run the entire test suite with coverage, it will definitely die. When I ran coverage reports every commit during the 8.x release cycle, I'd only run the unit tests because it doesn't really make sense to run the other tests. They don't provide @covers, and they don't cover specific pieces of code but features. Also, because like you'd have a million tests populating "coverage" for the bootstrap but none of them testing anything.

We actually do the filtering in core its just been renamed in xdebug 3 for obvious reasons.

  <coverage>
    <include>
      <directory>./includes</directory>
      <directory>./lib</directory>
      <directory>./modules</directory>
      <directory>../modules</directory>
      <directory>../sites</directory>
    </include>
    <exclude>
      <directory>./modules/*/src/Tests</directory>
      <directory>./modules/*/tests</directory>
      <directory>../modules/*/src/Tests</directory>
      <directory>../modules/*/tests</directory>
      <directory>../modules/*/*/src/Tests</directory>
      <directory>../modules/*/*/tests</directory>
      <directory suffix=".api.php">./lib/**</directory>
      <directory suffix=".api.php">./modules/**</directory>
      <directory suffix=".api.php">../modules/**</directory>
    </exclude>
</coverage>

Aside from all that, in general I wouldn't surprised if even unit tests for core had problems. I stopped running coverage reports because they'd constantly get broken because core wasn't running them so they don't get reviews and commits would constantly break them in weird ways. That's why I'm here! :-D

oh that looks promising. That does look a bit off but it's just their collection so shouldn't be a big deal.

Still agreed core needs a better setup in core but this should solve the immediate issue once its released.

Yeah, that looks right. I was suggesting we just add that environment variable to the default container config or the CI config since its unlikely anyone would want any other xdebug mode in CI.

on the php directives, looks like you where passing '-dzend_extension=xdebug -dxdebug.mode=coverage' to phpunit script but you'll need to pass them to php like php -dzend_extension=xdebug -dxdebug.mode=coverage ./vendor/bin/phpunit.

I suspect the Xdebug problem was just it running in the wrong mode. We could probably add the environment variable or set the default config to always run in coverage mode. I couldn't find the failed test though to confirm this.

Is there a advantage to adding pcov? Xdebug has always worked well enough for me.

Thanks! That's really helpful, I think I can take a first pass based on this.

On the last point, I think I understand I could test the pager block and that makes sense and seems like the right approach. I'm not sure that fully addresses my concern though. The ellipse could be failing but the test pass if the environment doesn't have enough content or configuration changed in a way as so the ellipse isn't rendered. Should it target the ellipse directly?

xjm → credited neclimdul → .

I didn't notice it in my initial review but with further testing I'm seeing some weirdness in javascript sorting. I think this might be causing problems with dependencies since they aren't being considered in the resort.

agreed

neclimdul → created an issue.

Opened a merge request that applies a similar patch to both the CSS and JS rendering.

I'm also not sure hot the aggregation/optimization interacts with this but based on some basic testing the aggregation did not seem to be affected by this sorting. That's to say, by using weight to place an inline element between two aggregated scripts, they where split into two aggregation files served before and after the inline javascript.

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

Feels like maybe the whole train of issues down that link are caused by this and the complexity it causes. Probably need to fix the underlying issue instead of chasing our tail trying to fix every edge case caused by it.

Ok, probably makes sense to have some support for the privacy stuff that is documented. "functionality_storage" "personalization_storage" "security_storage"

As far as doing this in the current advanced settings, I don't see a way atm. Are you suggesting adding something to the advanced settings to support it?

For reference, this came up implementing the suggested configuration for Osano.

https://docs.osano.com/hc/en-us/articles/22469433444372-Google-Consent-M...

neclimdul → created an issue.

Replacing a extension sounds pretty dangerous.

The current approach of calling \Drupal::service is functional but seems like a _huge_ performance hit going to the container ever template render.

Does anyone know why the deprecation compiler code was written into the extension? It seems like it might have just been convenient.

If it was important, maybe we can move it to a dedicated "extension" that's not really an extension that just provides this helper?

If the approach isn't important, maybe we can just inline the check. Something like this maybe:

    $compiler->write("isset(\$context['deprecations']) && array_map(");
    $compiler->indent();
    $compiler->write("fn (\$name) =>");
    $compiler->indent();
    $compiler->write(
      "isset(\$context['deprecations'][\$name])",
      "&& \array_key_exists(\$name, \$context)",
      "&& @trigger_error(\$context['deprecations'][\$name], E_USER_DEPRECATED),"
    );
    $compiler->outdent();
    $compiler->subcompile($usedNamesNode);
    $compiler->outdent();
    $compiler->raw(");");

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

This doesn't sound like a core bug. The addition of style sheets(and javascript) is all handled through libraries so if a webform component needs progress but isn't getting it then it just needs to add it to the list of required libraries in its render array or one of the other methods drupal provides.

https://www.drupal.org/docs/develop/creating-modules/adding-assets-css-j... →

Fair enough.

However, I don't think I've really got a firm answer on the correct approach and I had to modify the proposed resolution so before I go diving into tests I'm going to move this back to NR to get some review on the approach.

Took a first pass at the proposal in the IS. Don't know if that was correct or not.

Couple changes:

1. Since this is structured, I used aria-details instead of aria-described by per the note on the aria-describedby mdn documentation https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attribut...
2. Added aria-live to the dynamically changing region

I also had to mangle things a bit to push an id onto the suggestions but I think this might make things a bit better by not replacing the entire wrapper div. This might allow the aria-live to actually work.

neclimdul → changed the visibility of the branch 3272325-password-suggestions-are to active.

neclimdul → changed the visibility of the branch 3272325-password-suggestions-are to hidden.

neclimdul → changed the visibility of the branch 3272325-password-suggestions-are to hidden.

seems like with gitlab we could start moving toward just using phpunit directly? it would make this pretty trivial and could better integrate with all the other features gitlab provides

Don't know. We've been running this to great effect for close to 4 years now to and haven't seen a problem during that period and xmlsitemap never shows up on our cron metrics. Would be happy to help move this forward.

I haven't had any troubles with paragraphs but I suspect paragraphs is fighting with the mousedown behavior to work around core so there would need to be follow up I guess.

Yeah, using try/catch as a conditional doesn't feel great. I considered moving the regex to a constant we could use but that felt weird to so open to suggestions.

https://drupal.slack.com/archives/C1BMUQ9U6/p1709912605839939

Needs test but approach could use some review.

Realized my concerns about optimizing this for existing conditions is probably not a big deal. Core is passing urls and that's probably the common use case. fromUserInput is probably the expected behavior the rest of the time.

That said, we can avoid cloning is we just created the object so I've included that in the merge.

Needs tests but the existing twig extension test needs a lot of scaffolding for each test so I will come back to it.