🇺🇸United States @mglaman

Thanks, back-2-95!

mglaman → created an issue.

mikelutz → credited mglaman → .

Even better

Pushing back to NR to get a discussion on #37.

Thanks for opening, added notes there. Sorry, it's been on my list but kept getting buried.

Retitling. There should be generic test coverage around entity list builders implemented in Core to verify they're using the defined methods properly. Although I feel like this could be a PHPStan rule versus explicit test. "You didn't call the query method when loading entities" like the issue which spawned this.

Okay, this patch broke all the of the reason it exists – to avoid modifying the media library and others.

Well this patch causes toggle to just always apply versus staying as regular checkboxes and I have no idea if that's actually better or worse :/

Unfortunately I have no idea how that would be done. I'll see if I can find some CSS expertise on my side. Made a draft MR with a more blunt fix in the meantime.

mglaman → created an issue.

xjm → credited mglaman → .

mikelutz → credited mglaman → .

mikelutz → credited mglaman → .

It feels more of a coding standard / PHPStan rule than test coverage. After reading 🌱 [policy, no patch] Better scoping for bug fix test coverage RTBC it seems like this could be a bugfix without a test, but should have a follow up for preventing this kinds of changes in entity list builders.

Does the issue expose a general lack of test coverage for the specific subsystem? If so, is it better to add generic test coverage for that subsystem in a separate issue?

Also, this is true:

Is the fix achieved without adding new, untested, code paths?

Great, thank you!

We should implement the new interface and alias the old one.

Left a comment on the MR.

Came here to mention this. It was added in 📌 Use GitLab CI for testing Fixed without its own specific issue to explain why.

quietone → credited mglaman → .

We have https://www.drupal.org/project/jsonapi_resources/issues/3362189 📌 Implement ValueResolverInterface instead of ArgumentValueResolverInterface Active , please work on the specific fix there. It must also be backwards compatible.

Added test

As per usual, I forgot :) I'll see if I can get the test up today.

Taking a new approach with an route access policy that can be reused. Crediting tanmayk as he worked on a solution that inspired this approach.

This allowed my customizations to take effect

mglaman → created an issue.

tekNorah → credited mglaman → .

The test passed with this added before the a11y test was added there. It's a horrible chicken and egg problem of "What do we fix where?" We could fix this with the tests, but it seems like it should be its own fix. As far as I can tell, this issue fixes the visibility bug and can be RTBC'd and unblock testing the new Navigation module.

Okay, so it is not the current site's URL but intended to reflect whatever is the live/production URL for the codebase.

I think a 2.x would be best. Commerce Cart API isn't the "standard", whereas Commerce API is. So 2.x would reflect decisions made. There could be the problem where 2.x also replaces Backbone/Underscore with web components and Commerce API. Those are two big changes. But maybe that's just the way it has to go.

@matt: is the APP_URL for the current site? Or as a reference for the live site?

It's the current site. So example.com or uat.example.com.

You're meaning it should always be the production URL?

Seems reasonable. Canonical feels too developer

Laravel has this as well, `url`. It's used for CLI URL generation.

'url' => env('APP_URL', 'http://localhost'),

I'm sure other projects also have this. Would be nice for Drush when using the default site.

A discussion in ✨ Making Drupal automatically installable with default environment variables Needs work went from getenv to $_ENV. I'm somewhat indifferent; whatever makes this land faster. There are benefits to each. But if Drupal core is already using getenv then I'll switch

But this isn't about parsing or loading .env files.

This isn't about adding a package to load env files, but just allowing customizing the parameter without managing index.php yourself

@neclimdul good callouts, though. I opened ✨ Allow specifying the environment for the kernel using an environment variables Active to discuss the environment so we can do things like this.

mglaman → created an issue.

I mean even if we didn't, folks are using environment variables regardless. So the problem already exists that Drupal is providing this page. IMO it's more of a problem we don't really support APP_ENV and kill phpinfo for prod

$kernel = new DrupalKernel('prod', $autoloader);

to

$kernel = new DrupalKernel($_ENV['DRUPAL_ENV'] ?? 'prod', $autoloader);

And if prod, phpinfo page is turned off.

If there is a precedent now for guarding, I guess we can try. But it seems like a reach and bloat of scope/responsibilities.

Also pushed the axe test. It is failing. I think we should:

- Merge 🐛 Buttons are not accessible with display: contents applied Active to apply fix
- Follow @ckrina idea of making this a meta issue
- Commit the test site install script and expandCollapseTest.js (this issue or new child?)
- Make the child issues from #14 including a11y test

🙌 the Nightwatch test passed locally for me with these changes. I copied them over to ✨ Evaluate adding Nightwatch tests Active , waiting to double verify with pipeline https://git.drupalcode.org/issue/drupal-3393400/-/pipelines/169765

Okay, going to move to NW because this all kicked off since it's not accessible when testing with WebDriver and that needs to be verified. I'll be at a computer soon

@finnsky did you re-run the linked Nightwatch test? Or manually test.

@Gauravvvv thanks for adding a fix! I'll copy it to a test I have to verify it makes WebDriver happy

Found another report of this combo failing tests with Cypress: https://github.com/cypress-io/cypress/issues/25199

Marking for review, I have no idea the impact of the overflow. But from I've read, it isn't needed.

This failed

.admin-toolbar__scroll-wrapper {
  display: flex;
  /*overflow-x: hidden;*/
  overflow-y: auto;

@media (min-width: 64rem) {
  .admin-toolbar__scroll-wrapper {
    display: contents;
    background: none;

This also failed

.admin-toolbar__scroll-wrapper {
  display: flex;
  overflow-x: hidden;
  /*overflow-y: auto;*/

@media (min-width: 64rem) {
  .admin-toolbar__scroll-wrapper {
    display: contents;
    background: none;

This passed.

.admin-toolbar__scroll-wrapper {
  display: flex;
  /*overflow-x: hidden;*/
  /*overflow-y: auto;*/

@media (min-width: 64rem) {
  .admin-toolbar__scroll-wrapper {
    display: contents;
    background: none;

So it is not display: contents;. Only in combination with overflow

I've also found https://github.com/angular/protractor/issues/4951. There is no solution but some details to the problem.

Finally getting a chance to come back to this, and I was able to chisel it down quite a bit, and I've found that it's a combination of two style declarations: display: contents with overflow: hidden.

We're in that situation with

  overflow-x: hidden;
  overflow-y: auto;

Agreed. There's some CSS wrangling that will be needed in 🐛 Content preview blocks ability to click "Add block" link due to invisible menu items Active

I found https://github.com/SeleniumHQ/selenium/issues/6977 and their fix https://github.com/SeleniumHQ/selenium/commit/bbdf7c28a14645c3f5c53f2492...

      if (goog.string.startsWith(containerDisplay, 'inline') ||
          (containerDisplay == 'contents')) {

I don't exactly know the way forward, but it shows the fix is not removing display: contents but investigating the test issue

@mherchel, you're right. I took https://cdpn.io/aardrian/debug/NWqbggX and but the button inside of a div with display: contents. Maybe it's a bug in Chromedriver itself?

mglaman → created an issue.

It's due to the content preview. Quick fix is to turn it off and open a follow up for fixing.

I was able to get a screenshot showing the elements are blocking the click. I didn't catch what the CSS was.

Context for the error

          <div class="toolbar-popover__wrapper" data-toolbar-popover-wrapper="" inert="true">
                          <a href="/admin/config" title="Administer settings." class="toolbar-popover__header toolbar-button toolbar-button--large toolbar-button--dark" data-drupal-link-system-path="admin/config" tabindex="-1" data-once="toolbar-menu-link">Configuration</a>
                        <ul class="toolbar-menu toolbar-popover__menu">
                              <li class="toolbar-menu__item toolbar-menu__item--level-1">
                  <button class="toolbar-button toolbar-button--expand--down" data-toolbar-menu-trigger="1" aria-expanded="false" data-index-text="s" tabindex="-1" data-once="toolbar-menu-trigger">
            <span class="toolbar-menu__link-action visually-hidden">Extend</span>
            <span class="toolbar-menu__link-title">System</span>
          </button>
          <ul class="toolbar-menu toolbar-menu--level-2" inert="">
                            <li class="toolbar-menu__item toolbar-menu__item--level-2">
                  <a href="/admin/config/system/site-information" class="toolbar-menu__link toolbar-menu__link--2" data-index-text="b" tabindex="-1" data-once="toolbar-menu-link">Basic site settings</a>
              </li>
                    <li class="toolbar-menu__item toolbar-menu__item--level-2">
                  <a href="/admin/config/system/cron" class="toolbar-menu__link toolbar-menu__link--2" data-index-text="c" tabindex="-1" data-once="toolbar-menu-link">Cron</a>
              </li>
      
          </ul>

These are under Configuration but somehow being considered even though they're not visible

It failed on my first try:

WebDriver\Exception\UnknownError: unknown error: Element <a href="/layout_builder/choose/block/navigation/navigation.block_layout/0/content" class="use-ajax layout-builder__link layout-builder__link--add" data-dialog-type="dialog" data-dialog-renderer="off_canvas" data-once="ajax">...</a> is not clickable at point (131, 178). Other element would receive the click: <li class="toolbar-menu__item toolbar-menu__item--level-1">...</li>
  (Session info: headless chrome=124.0.6367.119)
  (Driver info: chromedriver=2.36.540469 (1881fd7f8641508feb5166b7cae561d87723cfa8),platform=Mac OS X 10.16.0 x86_64)

This may be caused by 🐛 Navigation layout Drag is not working in some browers Active .

I'm going to try and track it down locally while in the contrib room

The test is green, but:

- cherry-pick fix from 🐛 Buttons are not accessible with display: contents applied Active
- reverts 🐛 Fix z-index Needs review

This causes problems when trying to test the admin toolbar.

   An error occurred while running .click() command on <.admin-toolbar__expand-button[aria-expanded=true]>: unknown error: Element <button aria-controls="admin-toolbar" class="admin-toolbar__expand-button" data-once="admin-toolbar-trigger" aria-expanded="true">...</button> is not clickable at point (36, 750). Other element would receive the click: <div class="admin-toolbar__scroll-wrapper">...</div>
  (Session info: chrome=124.0.6367.119)
  (Driver info: chromedriver=2.36.540469 (1881fd7f8641508feb5166b7cae561d87723cfa8),platform=Mac OS X 10.16.0 x86_64)
    {"error":{"name":"WebDriverError","remoteStacktrace":""},"status":-1,"value":null}

mglaman → changed the visibility of the branch 3393400-nightwatch-tests to hidden.

Opened an MR that wholesale removes it, not sure the consequences

Opened 🐛 Buttons are not accessible with display: contents applied Active . Buttons are not accessible inside of display: contents;

I guess this is blocked on 🐛 Buttons are not accessible with display: contents applied Active , then.

mglaman → created an issue.

Buttons are not accessible with display: contents applied. See issues for Chromium, Firefox, and WebKit

Okay, I worked backward and added back styles. It is the display: contents

@media (min-width: 64rem) {
  .admin-toolbar__scroll-wrapper {
    display: contents;

We have a problem here. The scroll wrapper, .admin-toolbar__scroll-wrapper, causes WebDriver to think any elements within it are not visible for interaction. Oddly enough waitForElementVisible passes. Probably because the latter is inspecting the DOM and not interacting with it.

When I made these changes, the tests passed:

.admin-toolbar__scroll-wrapper {
  /*display: flex;*/
  /*overflow-x: hidden;*/
  /*overflow-y: auto;*/
  flex-direction: column;
...
}

@media (min-width: 64rem) {
  .admin-toolbar__scroll-wrapper {
    /*display: contents;*/
    /*background: none;*/
  }
}

I also needed to fix the footer's z-index in 🐛 Fix z-index Needs review . Once the wrapper adjustments were made, the footer was hidden behind the navigation.

I think this is a great idea. I had one piece of feedback for the implementation, then just some notes as I went through all of the comments.

Represent the component instance tree as two single-valued fields

Using JSON-based storage and storing the structure and data in two fields. However, I'd prefer one field with two properties. In the end its the same, two columns on the table. But we can mark the properties as internal with a computed property that is the combined value for building the page. This makes it API-friendly for frontend consumption.

Regarding storage optimization, it should be split out to not add complexity to this storage. Whether old revisions are garbage collected or their contents dumped and encrypted to disk and loaded if ever needed (I worked on this before), it should be solved elsewhere. We aren't making the existing problem worse by following normal paradigms. This means we can more easily solve the problem for the impacted areas. I'm on the same stance as Wim in #17.

Relies on JSON field storage which also doesn't quite exist yet at least in a battle-tested way.

RE #15, you mean battel-tested in Drupal core? Because folks have been using JSON fields with Drupal in production for some time.

Paragraphs are getting unfairly thrown under the bus here I think.

RE #22. I don't think its Paragraphs specifically. It's just the module that most easily highlights storage complications with revisions.

That's something which has always bothered me, why is it that base fields can quite happily just be columns on the same table, but each field created through the field UI must have its own table. (I guess that's why they're called base field heh)

That's because they're bundle fields and may or may not be used. I think it'd be worth opening (or maybe one exists) that any field with a cardinality of one for a bundle field can go onto the shared table storage and not a dedicated one. Because base fields move to a dedicated table when cardinality is > 1.

Correct, if someone used vlucas/phpdotenv without the PutenvAdapter there would be nothing returned from getenv. There are quirks. In Symfony they use $_ENV in DotenvVault

nicxvan → credited mglaman → .

nicxvan → credited mglaman → .

Started working on the tests, copied over @bnjmnm test https://git.drupalcode.org/project/drupal/-/merge_requests/7980

Experimental modules can't be installed using the install module command. since it won't be experimental for long, I just used a test setup script

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

It's ready for basic review. Test incoming

mglaman → changed the visibility of the branch 11.x to hidden.

mglaman → changed the visibility of the branch 3445816-making-drupal-automatically to hidden.

Opened MR. Screwed it up a bit since the issue targeted 11.0.x and not 11.x https://git.drupalcode.org/project/drupal/-/merge_requests/7970

I see. Thanks, $_ENV is localized to one process, whereas putenv is for the whole process.

as putenv()/getenv() are generally discouraged due to not being thread-safe

We can go either way - `$_ENV` or this. Honestly, I've never had a decent explanation for the thread-safe concern besides the footnote in the env library.

I feel like #2879846: Provide better support for environment variables → could solve this, but it is a bigger meta-question. So I think we can try and solve this particular problem. As long as the environment variable names do not change, the implementation can change.

This could be a duplicate of #2979749: Allow to pick up settings & DB creds from environment variables → , I think. I need to read through more. But there isn't a patch there either.

I've started working on changes to \Drupal\Core\Site\Settings::initialize and will have an MR during contribution sprints.

Adding thoughts on the approach.

  event_dispatcher.legacy:
    decorates: event_dispatcher
    class: Drupal\Core\LegacyEventDispatcher
    arguments: ['@event_dispatcher.legacy.inner']

👏 this is great

Assigning to myself for now, to see what I can hack on here at DrupalCon

mglaman → created an issue.

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

Could the original class be made to allow nullable constructors with this fix? It feels dirty. But I guess it's possible.