Nicaragua
Account created on 7 August 2011, over 12 years ago
#

Merge Requests

More

Recent comments

heddn Nicaragua

Nice work here. Thanks for the added tests.

heddn Nicaragua

Possibly if 📌 Additional source plugin filtering Needs review lands first, this could be closed duplicate and credit transferred over there.

heddn Nicaragua

Traced this back to 🐛 When adding a new menu link, restrict the available parents to the current menu Fixed . It isn't in 10.1 but is in 10.2. GCM does not have a full config backed menu. It never will. Additionally, the menu parent logic is overwritten by that module after this here. The patch in #2 seems like a decent solution. An entity load can fail to load (that's what we see here). We should check and handle appropriately.

Still needs tests, but this can be easily reproduced. In a test controller add something like:

  public function addLink(GroupContentMenuInterface $group_content_menu) {
    $menu_name = 'non-existent';
    $menu_link = $this->entityTypeManager()->getStorage('menu_link_content')->create([
      'menu_name' => $menu_name,
      'bundle' => 'menu_link_content',
    ]);
    return $this->entityFormBuilder()->getForm($menu_link);
  }
heddn Nicaragua

Is it just a change in format?

heddn Nicaragua

Tests pass green on php 8.2 in the bot. I couldn't figure out an easy way to run a test-only pass on the code to demonstrate the error w/o any fixes..

heddn Nicaragua
third_party_settings:
    sections:
        third_party_settings:
          lb_plus:
            uuid: 3084b21e-317d-44b3-9d74-5e3d126bdc0c

This is what is being complained about.

heddn Nicaragua

Line 22-24 is what is triggered. Solr has those values as null. But the validator thinks they should be an empty array.

    if (!is_array($value)) {
      throw new UnexpectedTypeException($value, 'array');
    }
heddn Nicaragua
+++ b/config/schema/fullcalendar_view.views.schema.yml
@@ -81,9 +87,21 @@ views.style.fullcalendar_view_display:
+      type: integer

Since this is a checkbox, this should be:

type: boolean

heddn Nicaragua

What version of config inspector? The most recent? None of my sites are still using 10.0/10.1 any more but when were still using it, we didn't have issues. It was only recently this started.

heddn Nicaragua

Automated testing in the CI where we essentially do protected $strictConfigSchema = TRUE does not trigger any exceptions. Running drush config:inspect --only-error and the web UI trigger the same exception logic.

heddn Nicaragua

I'm not sure how related or unrelated this is, but search_api_solr in 10.2 is throwing fits with the new config validation. 🐛 ValidKeysConstraintValidator thrown by config inspector Active has some details on what I'm seeing.

heddn Nicaragua

I think is comes from all of the nullable: true stuff all over the config schema in this module. There is a lot of it though, so I wonder if something in core should be handling this differently.

heddn Nicaragua

Not all custom code uses contrib entity module. The place that gets the greatest visibility is in core.

heddn Nicaragua

On 10.2+, the additional validation for ck5 attributes and properties is getting in the way. I attempted to use extend_html_filter and the the MR from Match the filter_html <> ckeditor5 integration in Drupal core Needs work and still no joy.

heddn Nicaragua

This MR fixes modules that use _this_ module's API. For group.module fixes, see the related issue.

heddn Nicaragua

This has test coverage added now. They were failing because they were actually written with the bug we are seeing present. There are 2 of the 19 different test cases in the data provider that included nested (below) items that are filtered out by the build process. But the twig variable was still indicating there were items.

heddn Nicaragua

A rebase should be all that is needed to fix the counts.

heddn Nicaragua

I've run into this before w/ drush on some other un-related operations. It assumes everything is non final and tries to wrap objects in its own wrapper via reflection. In the past we had to check for if the object was final. I wonder if that is what is happening here. The obvious thing is to not make the class final. But its awful convenient for a simple class like we have here to make it final and not need to worry about BC.

heddn Nicaragua

I fixed one small issue with a doc block. But that was so small I doubt it blocks from re-RTBCing. LGTM.

heddn Nicaragua

This is safe to land as dependency injection of the container should work even in earlier versions of Drupal.

heddn Nicaragua

Once support for 10.1 is sunset, this can land. Otherwise if we want to support both <=10.1 and 10.2+ at the same time, we'll have to do a BC dance.

heddn Nicaragua

Tests are mostly passing now on the MR. At least the same tests that aren't failing on HEAD are now passing. Hiding patches.

heddn Nicaragua

Tests were added in #42. I've rebased and moved the work into an MR since it wouldn't apply on 2.x HEAD.

heddn Nicaragua

The rebase fixing things is explained by 📌 Fix random performance test failures Needs review landing yesterday.

heddn Nicaragua

Looking what we are doing over in [##3402620], I think we need to do the same class exists check and link to the core issue.

heddn Nicaragua

Do we need to add the interface and the string strict typing here? Given the test failures, that feels like more disruptive then this was originally scoped.

heddn Nicaragua

This should fix the deprecation. And it should be safe to commit right away since entity loading has been around since 8.0.0.

heddn Nicaragua

Note, the change records also doesn't discuss any of the implications after upgrading to 10.2 and what that might mean to field widgets. See 🐛 Implement EntityReferenceItemInterface Needs work , where the States API totally breaks on form elements for fields now being nested in completely different locations.

heddn Nicaragua

And the CR reference See https://www.drupal.org/node/3364271 is for a node that 404s. Please, what do we need to do here? Open a follow-up to fix the CR and unify things?

heddn Nicaragua

Thank you for your effort to get this over the finish line. Much appreciated.

heddn Nicaragua

Are we supposed to review the patch or the MR now? I like what I see in the screenshot. Is that different then the MR or just a PoC and we still need to roll back into the MR? I'm confused.

Production build https://api.contrib.social 0.61.6-2-g546bc20