WI, USA
Account created on 12 December 2012, over 11 years ago
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

I tried approach 3 a bit more and worked around πŸ› Entity form default ::afterBuild and ::processForm are not working correctly for subforms Needs work . But it breaks down due to there being multiple subforms.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

I was hitting this. It's due to how ::CALLBACK is parsed. It is handled by \Drupal\Core\Form\FormStateInterface::prepareCallback. The problem is that it is the main form state executing this code.

  public function prepareCallback($callback) {
    if (is_string($callback) && str_starts_with($callback, '::')) {
      $callback = [$this->getFormObject(), substr($callback, 2)];
    }

Since it is not the subform state, there is always the incorrect form object. A fix would be to change ::processForm to [$this, 'processForm'] for a true callable.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

I tried out proposed option 2:

In Media source configuration provide a button to open the field settings form in a dialog for in-context modification.

It's not great. Saving the form in the dialog refreshes the page, which can cause a loss of data.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

I got this almost working, but it does bleed in concepts of field_ui. However, the submit is broken due to the way field config forms changed in πŸ“Œ Combine field storage and field instance forms Fixed .

On submit it breaks due to the entity builder associated to the field config form:

$form['#entity_builders'][] = 'field_form_field_config_edit_form_entity_builder';

Even though it is processing the subform, the entity being passed to the callback is the media type.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

mglaman β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

Thanks for checking @ptmkenny. Looks like it's mostly test related, thanks for BC shim for trait.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

Merged, tagging a release.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

Thanks for tidying up the MR.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

There are a lot of changes in this MR which didn't need to happen.

What is the breaking change? It looks like one or two test generator methods needed to be made static, correct? The MR you hid is far simpler and more succinct in changes needed: https://git.drupalcode.org/project/views_remote_data/-/merge_requests/18...

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

Thanks, back-2-95!

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA
πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

mglaman β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA
πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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?

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

Left a comment on the MR.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

This allowed my customizations to take effect

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

πŸ™Œ 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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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;
πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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)
πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

The test is green, but:

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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}

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

mglaman β†’ changed the visibility of the branch 3393400-nightwatch-tests to hidden.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

Opened πŸ› Buttons are not accessible with display: contents applied Active . Buttons are not accessible inside of display: contents;

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

I guess this is blocked on πŸ› Buttons are not accessible with display: contents applied Active , then.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

@media (min-width: 64rem) {
  .admin-toolbar__scroll-wrapper {
    display: contents;
πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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.

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

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

Production build 0.69.0 2024