🇺🇸United States @luke.leber

Here's something for us to run temporarily in the new year...

diff --git a/core/assets/scaffold/files/default.settings.php b/core/assets/scaffold/files/default.settings.php
index cd364bb00d..751a5de5ff 100644
--- a/core/assets/scaffold/files/default.settings.php
+++ b/core/assets/scaffold/files/default.settings.php
@@ -811,6 +811,14 @@
  */
 $settings['migrate_node_migrate_type_classic'] = FALSE;
 
+/**
+ * Aggregated asset error logging.
+ *
+ * This is used to inform site owners when CSS or JS assets fail to be included
+ * in an aggregated asset.
+ */
+$settings['log_aggregated_asset_errors'] = FALSE;
+
 /**
  * The default settings for migration sources.
  *
diff --git a/core/lib/Drupal/Core/Asset/CssOptimizer.php b/core/lib/Drupal/Core/Asset/CssOptimizer.php
index 8f2d910250..61d00ea129 100644
--- a/core/lib/Drupal/Core/Asset/CssOptimizer.php
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -5,6 +5,7 @@
 use Drupal\Component\Utility\Unicode;
 use Drupal\Core\StreamWrapper\StreamWrapperManager;
 use Drupal\Core\File\FileUrlGeneratorInterface;
+use Drupal\Core\Site\Settings;
 
 /**
  * Optimizes a CSS asset.
@@ -145,7 +146,11 @@ public function loadFile($file, $optimize = NULL, $reset_base_path = TRUE) {
     // stylesheets in their .info.yml file that don't exist in the theme's path,
     // but are merely there to disable certain module CSS files.
     $content = '';
-    if ($contents = @file_get_contents($file)) {
+
+    // Temporary hack for debugging impossible to diagnose aggregation issues...
+    $contents = Settings::get('log_aggregated_asset_errors') ? file_get_contents($file) : @file_get_contents($file);
+
+    if ($contents !== FALSE) {
       // If a BOM is found, convert the file to UTF-8, then use substr() to
       // remove the BOM from the result.
       if ($encoding = (Unicode::encodingFromBOM($contents))) {

Unrelated - there should probably be a strict comparison against the file_get_contents result here -- otherwise some false positives can slip through depending on the file contents 😱!

Ideally when we know that a corrupted asset is going to be generated, we'd want to issue an HTTP/500 to prevent the asset from becoming cached by downstream infrastructure, but that definitely can't happen without breaking huge swathes of sites.

Given the uncertainty about what's supported or not in #2, I think maybe we could add this behind an opt-in behavior to allow for a graceful path to deprecation?

I'm sure that a lot of sites out there have missing assets that's considered very, very normal.

I think this one might be dependent on how https://www.drupal.org/project/experience_builder/issues/3491021 ✨ Leverage HTML comment annotations, remove wrapping div elements Active pans out.

Hi all, given the relatively short remainder of life for Drupal 7, I'm going to close this out, but still assign issue credit where appropriate (hence the 'fixed' status) as it looks like genuine effort was put into this.

Cheers all.

luke.leber → changed the visibility of the branch 3351683-pp-1-resolve-all to hidden.

Hey, I'm closing this one as outdated, but if this is still an issue, please re-open it.

Thanks

I'm un-postponing this issue.

Given we're running on Gitlab now, let's get all of the jobs green. I'll open a fresh MR to start.

Hi Jane, that looks right for working with the Entity API in Drupal 8+. Thanks for sharing!

Hi, can you please provide the view configuration that you were seeing this on?

If we're not able to reproduce the issue with a carefully controlled set of steps to reproduce and write a new test for it, it's a lot harder to justify making any changes. I'm triaging this as PMNMI until we can hear more about the particulars of the site experiencing issues.

Thanks!

Good day, 4.0.0-alpha2 just landed. If you run into any issues don't hesitate to spin up an issue.

Thanks

Stand by for 4.0.0-alpha2 as soon as the main pipeline goes green.

I've manually tested the common use cases for this module against 11.1 and 10.4 including...

1. Media reference field configured to display juicebox formatters
2. Image field configured to display juicebox formatters
3. View configured to display content as juicebox galleries

Given we have green tests and a solid manual test, I'm going to merge this in and cut 4.0.0-alpha2 for folks waiting. If anything did manage to slip through, I'll be available to triage things throughout the week after the day job.

luke.leber → changed the visibility of the branch 4.0.x to hidden.

luke.leber → changed the visibility of the branch 3433346-automated-drupal-11 to hidden.

Green on 10.4.0, 11.0.8, and 11.1.0.

Testing on PREVIOUS_MAJOR, PREVIOUS_MINOR, and CURRENT seems very logical to me. Setting to Needs Review for manual testing.

This module does, indeed need work, as there are missing schema definitions.

Leaving NW so that someone can look into the test failures, or I can pick them back up when I'm able.

Outdated by https://www.drupal.org/project/juicebox/issues/3494657 📌 Get tests passing on Gitlab CI Active (oops, I forgot I already did this work last year :O ).

Now that I've merged this in,pulling in the 4.0.x branch on a dev site via composer require drupal/colorbox:dev-4.0.x will pull in the latest development build for the 4.0.x branch.

Subsequent patches can be applied on top of that. I'll see you over in the Drupal 11 issue.

The update bot has messed up the support ranges here.

The deprecation helper service was introduced in Drupal 10.1.3, but it has the module support range specified as >=10. That means that 10.0.0 through 10.1.2 users would have their sites killed by this.

The support range has to be at minimum ^10.1.3 || ^11 here. I strongly recommend using ^10.3 || ^11 since that's what we're able to test on though...

Setting back to needs work. If no one else picks this up before I'm able to, I'll make the commit myself, but in the meantime feel free.

Given these are test-only changes, I'll merge this in tomorrow evening if it hasn't been reviewed by then so we can move onto the Drupal 11 compatibility issue with confidence and get that one finished up by the weekend. 👍

FINALLY! A green run that actually runs the tests.

Setting to NR.

Hey, there are still some issues preventing this. Primarily not being able to run tests on Gitlab CI. Admittedly, I've slacked as a maintainer here, as I no longer manage any sites that actively use Juicebox, but I'll set aside time this through the weekend to hopefully grind through things and release something.

luke.leber → created an issue.

Isn't the required attribute the equivalent signal for non-visual users? It's announced by all major A/T tools.

Adding another signal is probably redundant IMO.

I gave !475 a test run, recompiled the UI, flushed some caches, and refreshed. Lo and behold, working required indicators.

This looks great to me, marking RTBC, as the accessibility concern is alleviated by this, even if a follow-up design task needs to happen.

luke.leber → created an issue.

This might be fine if we think SDC creators already know to output $attributes for other reasons, but #9 indicates that this isn't currently being consistently done in the wild.

We made the decision not to use $attributes in our twig, as it'd tightly couple things to Drupal. We may be the outlier, but I'm living proof of the concern :-)

Added copy/paste example to I.S. w/o any external dependencies :-).

It's actually pretty simple to resolve if the component knows if it's being observed in Experience Builder.

A custom twig extension...

class XBExtension extends AbstractExtension  {
  public function getFunctions() {
    return parent::getFunctions() + [
      new TwigFunction('is_xb_editing', [
        $this,
        'isXbEditing',
      ]),
    ];
  }

  public function isXbEditing() {
    return \Drupal::routeMatch()->getRouteName() === 'experience_builder.api.preview';
  }
}

...and using it in the SDC template:

{% set expanded = is_xb_editing() %} {# <-- "in_preview" variable-like-thing #}

{% include '@my-upstream-components/accordion.twig' %}

That said, I wouldn't recommend writing this off as a niche problem. I can't think of any interactive / progressive disclosure elements from *any* design systems that wouldn't need a mechanism like this. I think it's more along the lines of not having any example components available in XB to have stepped on this yet. It'd be really unfortunate for each design system to have to roll their own "in_preview"-like feature.

That's pretty neat, @jessebaker. It definitely *helps*, but the editing UX still seems off as the accordion seems to re-flip back to closed every time it's re-rendered.

Strangely enough, the newly added component seems to be selected by default even after re-paint and it's hidden.

luke.leber → created an issue.

Testing out the idea #32 is probably blocked by https://www.drupal.org/project/experience_builder/issues/3491021 ✨ Leverage HTML comment annotations, remove wrapping div elements Active .

I did play around with the latest work in #3491021, but it was too unstable confirm without any doubt that the approach is viable. From the limited experimentation I've performed, I think there's a good chance it'll be workable as #3491021 wraps up though.

Here's a gif of the behavior (described in previous comment).

Even when the accordion is collapsed, are you able to move a component into a hidden slot via the Layers panel (left sidebar)?

Yep. That works.

That should already work with #3486785: Initial support for allowing interaction with the preview. Not sure if state is retained when exiting preview though, or if the whole canvas gets re-rendered and therefore the accordion reverts to collapsed.

Yep, the preview mode works perfectly -- things are able to be interacted with just fine in there. It's just the non-preview mode where it's impossible to use the mouse and/or keyboard to expand/collapse interactive things.

While this can be worked around for accordions (through adding an "expand by default" prop and temporarily adjust that while adding content), I don't think it'll be quite as easy for other interactive UI patterns, like multiple tabs. For completeness, this is how we've set up our Tabs implementation in Layout Builder (dragging a block over a tab activator will actually activate the tab in the LB editor):

This all looks good to me! I re-ran

# Set a cache entry to persist for 5 minutes with a tag of "test_tag".
vendor/bin/drush eval "\Drupal::cache()->set('test', 'value', time() + 300, ['test_tag']);" && echo 'Set a cache entry'

# Confirm the cache entry is valid.
vendor/bin/drush eval "echo 'The cache entry is...' . (\Drupal::cache()->get('test') ? 'valid' : 'invalid');"

# Invalidate the tag via an ECA action.
vendor/bin/drush eca:trigger:custom_event test && echo 'ECA invalidated the tag'

# Confirm the cache entry is no longer valid.
vendor/bin/drush eval "echo 'The cache entry is...' . (\Drupal::cache()->get('test') ? 'valid' : 'invalid');"

with...

1. A single hard-coded tag
2. A single tag that was resolved by a token
3. No tags (invalidate everything)

I think this one's good to go! Great stuff!

Updated I.S. the xb link in toolbar works on HEAD now.

— Any chance you could add the missing test coverage? 🙏

All signs point to probably not 😭. NW and slowly backs away.

I suppose that an alternative approach might be to allow certain rendered elements receive input events, but that seems like it'd be quite the technical lift to accomplish.

Attached screenshot with more context.

However! when creating a brand new action (rather than starting with an old one, then patching), I get [warning] Undefined array key "backend" CacheActionBase.php:80 printed out.

I'm setting this back to NW. It looks like Drupal\eca_cache\Plugin\Action\CacheActionBase::getCacheBackend may need an update to cope with derivative classes that do not operate on a specific backend.

As a result, I'm pretty sure that Drupal\eca_cache\Plugin\Action::execute needs to be refactored to do a full cache flush and not exit early via

    if (!($cache = $this->getCacheBackend())) {
      return;
    }

That said, I'm not 100% sure what the right API calls should be in if tags are empty.

    if (empty($tags)) {
      $cache->invalidateAll(); <-- definitely not this, but I'm not sure what to replace it with!
    }

I was able to successfully test the invalidation action.

uuid: 46349853-db2b-4cfc-822c-9ac6680f2ff1
langcode: en
status: true
dependencies:
  module:
    - eca_base
    - eca_cache
id: process_jfh9njk
modeller: bpmn_io
label: noname
version: ''
weight: 0
events:
  Event_1bq8hvb:
    plugin: 'eca_base:eca_custom'
    label: Event_1bq8hvb
    configuration:
      event_id: test
    successors:
      -
        id: Activity_087tmd6
        condition: ''
conditions: {  }
gateways: {  }
actions:
  Activity_087tmd6:
    plugin: eca_raw_cache_invalidate
    label: Activity_087tmd6
    configuration:
      tags: test_tag
      backend: entity
      key: 'values:node:1'
    successors: {  }

# Set a cache entry to persist for 5 minutes with a tag of "test_tag".
vendor/bin/drush eval "\Drupal::cache()->set('test', 'value', time() + 300, ['test_tag']);" && echo 'Set a cache entry'

# Confirm the cache entry is valid.
vendor/bin/drush eval "echo 'The cache entry is...' . (\Drupal::cache()->get('test') ? 'valid' : 'invalid');"

# Invalidate the tag via an ECA action.
vendor/bin/drush eca:trigger:custom_event test && echo 'ECA invalidated the tag'

# Confirm the cache entry is no longer valid.
vendor/bin/drush eval "echo 'The cache entry is...' . (\Drupal::cache()->get('test') ? 'valid' : 'invalid');"

Output should be:

Set a cache entry
The cache entry is...valid
ECA invalidated the tag
The cache entry is...invalid

My only concern here is one of performance. If token substitution happens, but yields an empty string, does that mean that this kicks in?

When empty, then the whole cache is being invalidated.

For larger sites, full cache invalidations can lead to some pretty significant performance degradation, and in some cases denial of service if the ECA workflow runs often enough.

I'll put it on my list to do tomorrow.

luke.leber → created an issue.

luke.leber → created an issue.

luke.leber → created an issue.

luke.leber → created an issue.

luke.leber → created an issue.

I saw this today too, but chalked it up to me removing, adding, and refactoring props and slots on the fly without any regard for B/C.

For the most part XB seemed to be pretty forgiving, but I did run into this a couple times.

Manually visiting /xb/node/1 following step #7 is a winner though (fyi)!

luke.leber → created an issue.

Opened a MR based on #2, except the composer.json and *.info.yml support ranges match in !10. Both changed to ^10.2 || ^11.

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

I'm closing this issue out! Beta 15 and later seems to gracefully resolve this through https://www.drupal.org/project/entity_usage/issues/3366527 ✨ Differentiate pending/old revisions and default translations Active .

🥳

Drupal will generate an error on the Status report page (admin/reports/status) when a site is using "READ-COMMITTED" whilst having a table that doesn't contain primary key. The reason for this error is that "READ-COMMITTED" does not work as intended when missing primary keys

Say there is a table on the database that is not used by Drupal. Would that also fall into making things "not work as intended", or is the requirement check a bit overzealous in what tables it's checking?

Thanks!

luke.leber → created an issue.

Reviewing MR !25, none of this appears to be needed.

This iframe is marked display:none which effectively removes the DOM element from the accessibility tree.

The issue appears to be the result of naive accessibility scans and not the result of actual manual testing.

I recommend closing this issue, citing the display property on the iframe. If anything, I'd recommend removing the visibility CSS property as it's a no-op when paired with display none.

Marking as works as designed. If anyone has a11y citations suggesting otherwise, feel free to re-open, but all signs point to a buggy accessibility scanner as the cause here.

I know of at least 3 issues that were opened with confusion on this point. It may be a good idea to add some prominent documentation explaining this to the module page. $.02

+1 on deprecating the library.

The maintainer was explicitly presented with a dialog explaining what changing their username entails. They chose to do this despite the warnings about downstream chaos it would create.

To me, that means they probably shouldn't be an upstream dependency if such a change was made on a whim in this age of supply chain attacks.

I think that Jurgen is correct in #4. When invoking the Cache Tags Invalidator Service, that's a high level API that does not need to know / care about which backend is used.

All that needs to be passed to the service is an array of strings like node:## and the API should be able to do the rest.

Alright! I've worked through some testing with MR !36 and there's one tiny thing that needs to be added.

Here's the originally reported issue:

After applying the MR and re-loading the page, the YouTube thumbnail is still broken! This is because the render cache still contains a bad entry.

Following a manual cache flush, everything is looking good!

It looks like we need an empty post-update hook added to flush out any bad cache entries, and then this is good to go!

Thanks Felipe, the merge request looks good.

I'll give it a manual test on a real site later on this weekend and if all goes well, we should have a new release by Monday.

Cheers

Thanks for the report. I just hit up https://oembed.com/providers.json and /live is indeed a valid oembed endpoint scheme.

We'll have to add some test coverage and a fix to a merge request to get this one fixed, if anyone picks this up before I'm able to.

Cheers!

Chiming in as an early-ish adopter of content moderation. When we upgraded to Drupal 8, workspaces wasn't really a thing. We now have years of muscle memory built up around content moderation, having explained in depth to our users why it works the way it works as opposed to something like, say, the Oasis Workflow plugin for wordpress (which has been lauded above the Drupal tooling btw...).

In my experience, teaching the technical mechanics of a solution has been secondary to the "why doesn't this work like other workflow solutions that I am used to" barrier.

For us, it would be a very hard sell a switch to something completely different again with different justifications involved. If a motion to strong-arm use of a specific workflow tool is inevitable, I would urge looking at the most popular (by number of installs globally) workflow tools and to derive the end-user facing terminology from those tools versus Drupal implementation detail nomenclature.

Not sure if this is helpful at this stage of the game or not, but I wanted to get this point of view out there.

What is the benefit of changing the name of the spaceless filter in a non-bc way?

Maybe I'm dense, but not causing excess contrib and custom disruption should be a chief consideration in the decision making process, right?

gábor hojtsy → credited luke.leber → .

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

Re- #58

See https://fiddle.nette.org/twig/#3c6dbf089c

There is certainly changes in the generated output such that whitespace control operators are not a direct replacement for spaceless.

catch makes a good point in #56. The if statement could probably just be a version check on the twig dependency.

My main point here in #55 is that we probably shouldn't reimplement spaceless with a new filter name, as that would just cause needless untold fatal errors in custom and contributes themes on upgrade where it could otherwise be avoided if the same filter name is used.

Agree with #14. MR !9329 is very close to what we use in our custom theme, but I'm not convinced it's core-worthy. It's pretty messy and would be hard to predict / guarantee b/c for folks that do strange things to their messages templates.

There would have to be at minimum additional API set up to dynamically fetch the supported status headings in order to even offer full backwards compatibility support.

Is there potential here to approach this a bit more seamlessly?

For example, can we detect if spaceless exists upstream and only if it does not, add a twig function that matches the contract 100%?

  if (upstream spaceless does not exist) {
    // Replace removed upstream spaceless filter with our own.
    new TwigFilter('spaceless', [self::class, 'spacelessFilter'], ['is_safe' => ['html']]),
  }

This would mean zero disruption to downstream clients.

This is eventually going to break quite a few, if not the majority of custom and contributed Drupal themes.

Is this something that automation will be able to help with? Just a couple years ago, people were explicitly advised to use the filter, as the tag was going away.

Our search specialist is out sick, but we've not been able to add an additional index to any of our environments due to what seems to be an error in the Acquia dashboard.

We're going to bring it up on our TAM call tomorrow, but according to what we've read in the upgrade notes, preparing a new index will be critical to ensuring a no-downtime upgrade due to https://www.drupal.org/project/acquia_search/issues/3403480 💬 Support for Search API Solr 4.3.x Active .

More details to come.

I can ask our search specialist to take a look at the changes and give things a test drive. We have quite a bit of other stuff going on, but there may be down extra hours.

Can we get a "cut a 3.2.0 issue" spun up so he can report the feedback on it?

Thanks

Issue credit should be given to Benji Fisher too, as he performed a critical review.

That's a clever use of drupal_static.

Everything looks good to me here. Our team reviewed the functional bits yesterday fairly comprehensively in a code review.

Thanks!

Setting back to NW for test fails, but !41 seems to do the trick, functionally.

This sounds a lot like a very rare and hard to reproduce issue we've seen on our production Acquia sites. In our case, this manifested as the incorrect revision being marked as the default revision, and showing stale information to end-users on the front-end.

Following, and will try to reproduce in stage! Thanks for filing this issue! It could help to explain the "that can't happen" thing that certainly happens 🤣!

Hey, Jakob.

I'll take a look and see if we can apply the patch for tomorrow's deployment and gather some feedback from the ones generally affected by slowdowns on deployments.

I'm very confident that this will make a world of difference for our staff especially.

Thanks!

Our TAM suggested that the engineers on this issue might want to connect in order to iron out easier steps to reproduce the issue, so that a potential fix can be confirmed.

In order to test this issue, make sure the following preconditions are met:

1. You will need four or five different authenticated users in anonymous sessions (attempting to test this with a single user will be thwarted by PHP session locking!). Incognito tabs can work for this.
2. A patch to mock a slow response from Acquia's search service (follows).
   

   diff --git a/src/AcquiaSearchApiClient.php b/src/AcquiaSearchApiClient.php
   index 4bb268e..bc34604 100644
   --- a/src/AcquiaSearchApiClient.php
   +++ b/src/AcquiaSearchApiClient.php
   @@ -211,6 +211,19 @@ class AcquiaSearchApiClient {
       *   Response array or FALSE.
       */
      private function searchRequest(string $path, string $query_string) {
   +
   +    // !!! DO NOT COMMIT THIS !!!
   +    // Simulate a slow response from Acquia's search service.
   +    //
   +    // This is an exaggerated time, but makes it easier to see the underlying
   +    // problem.  This simulates a blocking HTTP call that is hanging.
   +    sleep(30);
   +
   +    // We don't actually want to call the real search service, since the goal
   +    // of what we're doing is to simulate a worst case scenario without any
   +    // real world conditions to contend with.
   +    return FALSE;
   +
        $subscription_data = $this->subscription->getSubscription();
        // Return no results if there is no subscription data.
        if (!$subscription_data || !$this->subscription->isActive()) {
       

Once signed into all of the various user accounts and the simulated enabling criteria patch is in place...

1. Flush the drupal cache
2. Visit /admin/config/search/search-api/server/acquia_search_server on one account. This will be the process that obtains the lock. It will hang for 30 seconds before releasing its lock and failing gracefully.
3. At the same time that the first request is executing, fire off an additional request from each different incognito window to /admin/config/search/search-api/server/acquia_search_server. This simulates a backlog of requests that would ordinarily be generated by users on a production site.

4. Note that *all* of the processes will be blocked until either they either...

   1. Obtain the lock, spins for an additional 30 seconds, and fails gracefully
   2. - or - fails to acquire the lock and fails with an exception

   In either scenario, the processes will just hang for excessive amounts of time. On a real production site, there are a finite number of php processes available. Eventually resources will be exhausted and end-users will be affected.

If more information, or a live demonstration is desired, please reach out to me and we can set up a call.

Thanks.

Hi ananya, thanks for the comment, but there are a couple problems:

1. Drupal.org only supports merge request workflows now [in order to run automated testing].
2. The problems with the 2.1.x branch are documented in the issue summary. Changing the version identifiers is not a solution here, as there are real test failures to resolve.