🇺🇸United States @luke.leber

Pennsylvania
Account created on 1 December 2016, about 8 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States luke.leber Pennsylvania

Setting back to Needs Work. Here's a slack chat to document where things are.

balintbrews
23 minutes ago
@lleber
I might be doing something wrong, but I can’t reproduce the issue on 0.x anymore.

lleber
:male-scientist: 23 minutes ago
I'll double check, but I was just able to around 8:30 :sweat_smile:

lleber
:male-scientist: 16 minutes ago
Ah, so it may be a dependency based thing :open_mouth:.
I added an example-flexbox-break component and here's the CSS inheritance...

(XB styles override example-flexbox-break component styles here)

9:38
Then I added a header (custom) component and here's the inheritance!

(header component styles override XB styles here)

9:39
The [data-xb-slot-id] selector seems to have nondeterministic loading order, meaning the example in the I.S. seems to work, but another component does not!
9:40
I'll dig a little deeper to see if I can sleuth a better example.

balintbrews
13 minutes ago
I can see that you have CSS aggregation turned on. Maybe that comes into play as well?

lleber
:male-scientist: 13 minutes ago
I can try disabling it, but the only reason the example component in the I.S. seems to work is because its display: flex is being overridden.

balintbrews
13 minutes ago
Yes, I saw that happening on my end as well.

lleber
:male-scientist: 12 minutes ago
I do think that if the SDC had a dependency injected on the preview CSS, it'd straighten itself out, but that seems odd.

balintbrews
12 minutes ago
Yeah, we don’t want that.
9:44
@lleber
I’m leaning towards just committing the width: 100% you suggested, I can’t see how that would hurt us. And it may help you now. :slightly_smiling_face: If we see more of these styling issues, we can do a deeper investigation. What do you think?

lleber
:male-scientist: 8 minutes ago
I think it'd definitely help in the short-term, but it may be a harbinger of deeper issues. I think it has to do with the SMACSS layer the libraries are loaded at.
9:46
The component displaying problems is registered at the component layer. Trying to figure out which layer SDCs are registered at.
9:46
I wonder if the XB styles should load at the lowest layer so that everything can override them? Or will that cause issues (see: layout builder off-canvas :smile:)? (edited)
New

balintbrews
2 minutes ago
Our styles in preview.css are unlayered, so they will take precedence over other layers.

lleber
:male-scientist: 1 minute ago
Hm..then I wonder how our custom header is winning there. I'm out of time this morning to debug, but I'm unblocked to find a root cause now.

I think a deeper investigation is needed to see what exactly is going on.

🇺🇸United States luke.leber Pennsylvania

This is certainly still an issue, except now the .xb--sortable-slot-empty-placeholder class doesn't specify a minimum width, and will collapse down to 0px wide if the parent element has display: flex.

I think that the safest bet here is to give the .xb--sortable-slot-empty-placeholder a width of 100%. The rationality here being that there should only ever be one empty placeholder per slot, right? As soon as a component is added, the placeholder goes away. I can't think of any reason why we wouldn't want the placeholder's drop-zone to be smaller than it could be!

🇺🇸United States luke.leber Pennsylvania

https://www.drupal.org/project/experience_builder/issues/3491021 Leverage HTML comment annotations, remove wrapping div elements Active has landed and this issue needs re-triaged. Self-assigning and will get around to this tomorrow between 8:30 and 5:00 ET.

🇺🇸United States luke.leber Pennsylvania

Whenever I see a call to wait on a lock I get a chill up my spine.

From a cursory glance, this change could result on a ton of processes spinning/waiting to acquire a lock under a heavy load, couldn't it?

I think this one should be subject to some load testing on a larger site.

🇺🇸United States luke.leber Pennsylvania

A first step is probably to get all of the changes needed in place that Gitlab needs to get all of the jobs green. That's a standardized set of tooling that everyone can share. I think https://www.drupal.org/project/juicebox/issues/3351683 📌 [PP-1] Resolve all coding standard violations Postponed is the place for that work I think. Any commit made to the issue fork over there will automatically run those tests.

Koustav Mondal self-assigned that issue to them self, but hasn't made any commits in a week, so IMO it's fair game for anyone to commit to.

Things like phpstorm configuration will probably vary from person to person, but the gitlab tests are a shared configuration between everyone. They should probably be resolved first before making any other changes so that everyone's on the same page with the standard checks.

After the gitlab checks are all passing, so long as further changes don't make the checks fail, they can probably be applied in a follow-up.

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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

Thanks

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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!

🇺🇸United States luke.leber Pennsylvania

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

Thanks

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

FINALLY! A green run that actually runs the tests.

Setting to NR.

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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!

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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

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

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

Attached screenshot with more context.

🇺🇸United States luke.leber Pennsylvania

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!
    }
🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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 .

🥳

🇺🇸United States luke.leber Pennsylvania

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!

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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!

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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!

🇺🇸United States luke.leber Pennsylvania

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.

🇺🇸United States luke.leber Pennsylvania

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?

🇺🇸United States luke.leber Pennsylvania

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

🇺🇸United States luke.leber Pennsylvania

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.

Production build 0.71.5 2024