🇺🇸United States @danflanagan8

This is biting me in 1.1.1

I just opened 🐛 Media library "Insert Selected" can still become impossible to scroll to Active

danflanagan8 → created an issue.

I can't seem to get the pipeline to run in Gitlab. Getting a console error and nothing happens. :shrug:

I can't say I remember working on this one, but it looks like I was pleased with the test in #8, which was included in the MR created by @joegraduate.

Looking at the test only fail in Gitlab I see (from 11 months ago):

1) Drupal\Tests\migrate_plus\Kernel\Plugin\migrate\process\EntityLookupTest::testLookupEntityWithoutBundles
Failed asserting that false is null.
/builds/issue/migrate_plus-2830058/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/builds/issue/migrate_plus-2830058/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:56
/builds/issue/migrate_plus-2830058/tests/src/Kernel/Plugin/migrate/process/EntityLookupTest.php:110
/builds/issue/migrate_plus-2830058/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

That doesn't look like too much of problem, eh? I wonder if the original failure (now lost to time) was more of an explosion than that. I wonder if something hasn't changed in this plugin since October 2022 that might make it handle null without blowing up.

There's quite a bit of action on this plugin in the git history: https://git.drupalcode.org/project/migrate_plus/-/commits/6.0.x/src/Plug...

I'm going to retrigger the pipeline in gitlab and see what it looks like.

smustgrave → credited danflanagan8 → .

After some testing in the wild, we determined that the caching of the redirect was problematic. We don't want it anywhere in the Drupal cache or the browser cache, otherwise the session might not get started when the user tries the preview link.

D11 compatibility patch attached.

I also wanted to report back on content moderation. I added an event subscriber to my custom codebase.

<?php

namespace Drupal\my_module\EventSubscriber;

use Drupal\scheduler\Event\SchedulerEvent;
use Drupal\scheduler\Event\SchedulerNodeEvents;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

/**
 * Special sauce for scheduler events.
 */
class MyModuleSchedulerSubscriber implements EventSubscriberInterface {

  /**
   * Sets the (un)publish_state values.
   *
   * This makes scheduler_repeat compatible with content moderation in
   * two ways. First, it (re)sets the pubslish_state and unpublish_state
   * values, which the scheduler_repeat does not do for us. Second, it
   * sets the current moderation state to draft; at this point the node
   * would be in the archived state and it can't go straight from there
   * to published.
   *
   * @param \Drupal\schedulder\Event\SchedulerEvent $event
   *   The scheduler unpublish event.
   */
  public function onUnpublish(SchedulerEvent $event) {
    $node = $event->getNode();
    $needs_save = FALSE;
    if ($node) {
      if (isset($node->publish_on->value)) {
        $node->set('publish_state', 'published');
        $node->set('moderation_state', 'draft');
        $needs_save = TRUE;
      }
      if (isset($node->unpublish_on->value)) {
        $node->set('unpublish_state', 'archived');
        $needs_save = TRUE;
      }
      if ($needs_save) {
        $node->save();
      }
    }
  }

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    $events = [];
    // We need this to run after scheduler_repeat, thus -10.
    $events[SchedulerNodeEvents::UNPUBLISH][] = ['onUnpublish', -10];
    return $events;
  }

}

I decided to open up an issue against wse so that I could put in an MR to adjust the constraint. ✨ Modify constraint to allow new ignored entities Active

danflanagan8 → created an issue.

@amateescu, thanks for the quick reply!

The error I'm seeing is "Paragraphs can only be created in the default workspace.", which is coming from the WseEntityReferenceSupportedNewEntitiesConstraint, though the core one would flag this as well.

The key checks are...

From core:

if ($value->hasNewEntity() && !$this->workspaceInfo->isEntityTypeSupported($target_entity_type)) {
      $this->context->addViolation($constraint->message, ['%collection_label' => $target_entity_type->getCollectionLabel()]);
    }

From WSE:

if ($value->hasNewEntity() && !$target_entity_type->isInternal() && !\Drupal::service('workspaces.information')->isEntityTypeSupported($target_entity_type)) {
      $this->context->addViolation($constraint->message, ['%collection_label' => $target_entity_type->getCollectionLabel()]);
    }

I guess the situation is that I could create a Paragraph in a Workspace if I somehow do it without using the Paragraphs field widget. So technically, CRUD is allowed, but in practice creating a Paragraph is going to throw this constraint validation.

On my client's project, I could do a field_info_alter to remove this constraint from paragraph fields. But is this in fact a bug in the validation constraint?

Interested in this...

I was testing this patch out and it didn't seem to change anything for me. I thought it was maybe because the Paragraph entity type has a php attribute (or whatever) now in addition to the annotation that the patch modified. More likely it's that I have wse installed and that module alters entity type info for several common entity types:

/**
 * Implements hook_entity_type_build().
 */
function wse_entity_type_build(array &$entity_types) {
  // Allow CRUD operations for various entity types in workspaces.
  $ignored_entity_types = [
    'crop',
    'events_logging',
    'file',
    'paragraph',
    'variant',
  ];
  foreach ($ignored_entity_types as $entity_type_id) {
    if (isset($entity_types[$entity_type_id])) {
      $entity_types[$entity_type_id]->setHandlerClass('workspace', IgnoredWorkspaceHandler::class);
    }
  }
}

This still doesn't all the Create part of CRUD though, right? I can only create a new entity in a workspace if it `isEntityTypeSupported` which has to pass this check:

$supported = !is_a($entity_type->getHandlerClass('workspace'), IgnoredWorkspaceHandler::class, TRUE);

So RUD operations seems to be more accurate.

RE: #12, my clients are pointing out the same thing. I think it is the responsibility of this module to not override the core workspace styling. I created an issue and put in an MR.

🐛 Environment indicator overrides workspace toolbar style Active

danflanagan8 → created an issue.

danflanagan8 → created an issue.

I'm coming around to thinking there are too many edge cases to try to get this to work well in the processor.

I think I'm going to just use the "Exclude specified items" processor configured to use a regex and exclude any value that is all numbers. (And I'll be careful to run it after the entity id /entity label conversion).

danflanagan8 → changed the visibility of the branch 2.0.x to hidden.

I'm running into this right on a project using a 3.x release. If the maintainers want to close this as won't fix, that's ok, but I think outdated is inaccurate. I'm going to reopen.

Exposing the ids of deleted entities is unexpected and undesirable behavior. We should fix the processor to unset the dead facets by default. I don't think we even need to expose a way to continue the current behavior. I can't imagine anyone liking it the way it is.

I put in an MR. Setting to NR.

danflanagan8 → created an issue.

Adding related issue that (I think) necessitated the updates to the libraries definitions

Almost certainly related to 🐛 Logic error in Drupal's lazy load for asset aggregation Active , which was introduced in 10.4.0.

Thanks for committing, @lozbes.

Could I kindly request that you add credit for @nikhil_drupal? (It doesn't happen automatically anymore.) He's a good dude and this looks like his first ever patch/MR to have been committed. Nice work, Nikhil!

Cheers!

The patch stopped applying because the use statements were re-ordered to comply with code standards in 📌 Enable GitLab CI automated testing Active .

The patch fails to apply to 3.3.0.

Here's a patch with with the change I mentioned in #30.

I have confirmed this buggy behavior on Acquia Cloud with Varnish. If anyone needs an immediate fix, we can always cache bust by adding a random query string to the end of the url. That's a lot to ask of previewers, though, especially if they're going to be clicking around to multiple pages during the preview. But in a pinch!

I like the idea of starting a session in order to bypass Varnish (or other reverse proxy caches). I made a patch that I tested in an Acquia Cloud dev environment. It seems to have done the trick.

I'm going to put in an MR. I'm going to start lazy and eschew DI. If the maintainer approves the approach, we could then polish it.

This would be a great feature, to be sure. I mostly like the implementation in the patch in #28. I'd probably rather have less in the module file and more in a service, but that's no big deal.

The patch didn't work for me though. I had to add a $node->save(); near the end of Drupal\scheduler_repeat\EventSubscriber::unpublish(). I'm using scheduler 2.x. Scheduler itself saves the unpublished entity before dispatching the unpublish event, so we have to save it ourselves if we want our changes to persist.

As for #29:

Does this patch support content moderation?

Short answer is no. Long answer is that scheduler always needs the Scheduler content moderation integration → module to work with workflows. Even with that module enabled, the repeater doesn't quite work. The reason is that the publish_state and unpublish_state get reset on publish and unpublish respectively by the scheduler_content_moderation_integration module which means they cease to be available for the scheduler_repeater module.

So that's a pain. Getting this to play nice with scheduler_content_moderation_integration might require a patch over there.

After adding externalauth and file to $modules in my failing Kernel tests, the tests are green again.

As @dpi wrote in #29:

It is only by luck (the service provider) that you havn't needed it.

My luck ran out!

I don't know enough about the module to say whether MR144 is a good idea regardless, but my problem was that I wasn't careful enough with dependencies in my Kernel tests.

@pfrilling It's hard to say for certain because I don't have any way to authenticate locally through openid_connect. What I can say is that for anonymous users locally or users logged in via `drush uli`, the site seems to be ok. No failures in my pretty significant suite of ExistingSite tests. I don't know how definitive that is though.

So I suspect this is just a Kernel test problem. I don't understand autowiring at all, and I'm also shaky on how Kernel test handle the container and services, but I suspect it's somehow the way that that all interacts in Kernel tests. :shrug:

I have the same exact problem as @anna d in #20. My Kernel tests are hosed whenever I have `openid_connect` in $modules using alpha6.

baikho → credited danflanagan8 → .

I ran into this a couple days after you and thought it was something with bef. I put in this issue: 🐛 Collapsible checkboxes filter doesn't stay open if only first option is checked Active

Then today I tried to write tests for that issue and couldn't reproduce it! I figured out that Core exposed filters set a #default_value to an empty array while the new facets exposed filter plugin leaves the #default_value as NULL.

Then I found this issue. And here I am.

The patch always sets [] as the default value. Do we need something else if multiple selections are not allowed? An empty string maybe?

I couldn't reproduce this with just bef enabled and figured out that this is instead a problem with the exposed filter plugin from facets 3.0.x.

🐛 BEF Collapsible Filter Not Automatically Opening For First Option Active

Instead of setting the #default_value to an empty array, it leaves the #default_value as NULL. Core exposed filters set the `#default_value` correctly and there is no problem.

Closing. Thanks!

MR ready for review. I added test coverage and my manual testing was successful. I'm less sure about the form UX and labels, etc. That's the hard part!

danflanagan8 → created an issue.

I haven't tested on 7.x, but the code's the same.

danflanagan8 → created an issue.

I'm working on converting all my facets to better exposed filters.

I tried out views_dependent_filters and it didn't work at all for me. It looks like maybe it was good in D7. It's not a viable solution in D10/11.

I solved the issue for myself by extending the bef Radios/Checkboxes filter and using it for my facets.

    // Remove corresponding details if no options.
    if (empty($form[$field_id]) && empty($form[$field_id . '_wrapper']) && isset($form[$field_id . '_collapsible'])) {
      unset($form[$field_id . '_collapsible']);
    }

This also checks for the `_wrapper` element as @johnny5th suggested might work.

I'm also using the extension to manage the conditional visibility of facets, which is already a won't fix in bef: ✨ Add support for conditional exposed filters in views Closed: works as designed

I ran into this exact problem. I'm upgrading all my facets blocks to native exposed filters in facets 3.0. The plan there is to lean heavily on this excellent configurable_views_filter_block module. I'm wanting to use collapsible bef filters and experienced the same thing in the IS.

I left a couple comments in the MR. There are a couple of if conditions that seem a bit off.

That said, the patch works for me and I'm going to use it. (Thanks, @mlncn!)

The concerns I have with the if conditions aren't going to affect my site. I do think the fix would be more robust if the conditions were modified slightly per my comments though.

I don't have permissions to manage releases on this project. I'm a lower-level maintainer of this module.

We need @dom. to cut a new release.

I think both code review (the R in RTBC) and manual testing (the T in RTBC) need to be done by one or more people in the Community (the C in RTBC).

This is an old issue, too, so it would be great to turn the patch into an MR so that the tests can run (assuming the project is configured to run tests in Gitlab).

I'm no longer using this module and I don't have a lot of contribution time at the moment. I probably won't be able to offer too much support. The issue has 6 followers, though, so maybe it will get over the finish line yet!

Cheers!

> does the current implementation avoid creating map and message tables?

Yes, the module provides an id_map plugin that extends the null plugin.

https://git.drupalcode.org/project/migrate_sandbox/-/blob/1.1.x/src/Plug...

It spits messages out with the Drupal messenger, but nothing goes to the db (he says as he proudly points at test coverage).

Very interesting bug, @benjifisher. menu_link_parent is an odd duck. I never added demo configuration for this one. I wonder if that was because I tried and ran into this and gave up? More likely it looked like it wouldn't work in the sandbox and I never even tried to come up with a demo.

Making the process plugin itself configurable would (probably) allow things to work without failing.

How would we fix this withing Migrate Sandbox...

There is no migrate_sandbox migrate plugin. That's kind of by design in that this is trying to be very separate from everything else. But we're seeing that there are drawbacks to circumventing the plugin system a bit. We end up getting an executable without using the migration plugin manager. The funny business happens around here: https://git.drupalcode.org/project/migrate_sandbox/-/blob/1.1.x/src/Form...

One thing I could do is look at making this all cleaner.

But I'd bet we could add a very simple stubby migration plugin to the module and trick this process plugin into running without failing.

Thanks for the clear reply, @stefan.korn. That scenario makes a lot of sense and seems like really good UX for the end user.

I'll assign this issue to myself to get some tests written.

Hi, @stefan.korn. The MR looks really good. I really like the use of #states on the settings form. I would be happy to update the tests to cover this new feature.

Out of curiosity, would you describe the use case where setting force_download_filename was a requirement?

I tested this and it works. I'm just going to merge and do a release.

Ah dammit. I committed to the repo and not the issue fork. I reverted it. Trying again on the fork.

I put in an MR. I made the category "viewfield" so that it doesn't get buried in the "reference" category.

There was never a rector rule for this, AFAICT. I found a discussion in Slack, but I don't see anything in the rector issue queue.

https://drupal.slack.com/archives/C03L6441E1W/p1716293607661759

Huh, when I run the test suite locally on D10.3 I indeed get these deprecation notices:

Remaining self deprecation notices (16)

9x: Using a translatable string as a category for field type is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3375748 →
1x in ViewfieldFormatterTest::testViewfieldFormatterDefault from Drupal\Tests\viewfield\FunctionalJavascript
1x in ViewfieldFormatterTest::testViewfieldFormatterTitle from Drupal\Tests\viewfield\FunctionalJavascript
1x in ViewfieldFormatterTest::testViewfieldFormatterRendered from Drupal\Tests\viewfield\FunctionalJavascript
1x in ViewfieldFormatterTest::testViewfieldArgumentHandling from Drupal\Tests\viewfield\FunctionalJavascript
1x in ViewfieldFormatterTest::testViewfieldItemsToDisplay from Drupal\Tests\viewfield\FunctionalJavascript

why don't I see those in Gitlab?

I was wondering why the viewfield tests don't fail on D11. I think it's because this bug is only triggered when using field UI to add a field. In the tests, the fields are created programmatically. But then why do the same tests pass on D10? Shouldn't there be some kind of deprecation notice that pops up in D10? Or shouldn't there have been a rector rule for this change? This looks like a change we should have handled better as a community.

I did a quick search for that same error and have found some related issues:

🐛 TypeError: Cannot access offset of type Drupal\Core\StringTranslation\TranslatableMarkup in isset or empty in Active
🐛 Changes in site building process can run into Cannot access offset of type StringTranslation\TranslatableMarkup on field creation path Active
🐛 Site crush when creating new fields Active
🐛 FieldItemBase category cannot be translatable Active

I haven't looked closely, but the last one in particular seemed to be looking at the field type category as I had suspected.

I wonder if this has anything to do with recent changes to how field type categories work. See this change record: [#3375748].

What happens if you hack the viewfield module and change the field type category from @Translation('Reference') to "reference"?

Here's the line: https://git.drupalcode.org/project/viewfield/-/blob/8.x-3.x/src/Plugin/F...

danflanagan8 → changed the visibility of the branch 3488780-formatter-wsod to hidden.

I created 🐛 ALA field formatter causes WSOD Active

I'm setting the status of this issue to Fixed since the MR has been committed. Let's focus on the new issue.

danflanagan8 → created an issue.

Even a basic attempt at testing this code would have shown that is fails catastrophically.

The website encountered an unexpected error. Try again later.

TypeError: Drupal\ala\Plugin\Field\FieldFormatter\AdvancedLinkAttributesFieldFormatter::_construct(): Argument #1 ($token) must be of type Drupal\Core\Utility\Token, string given, called in /app/docroot/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php on line 40 in Drupal\ala\Plugin\Field\FieldFormatter\AdvancedLinkAttributesFieldFormatter->_construct() (line 50 of modules/contrib/ala/src/Plugin/Field/FieldFormatter/AdvancedLinkAttributesFieldFormatter.php).

The dev branch is currently unusable.

danflanagan8 → created an issue.

Looks lik the LB failure is a known rando:

see 🐛 [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing Active or 🐛 [random test failure] Drupal\Tests\layout_builder\Functional\LayoutBuilderBlocksTest::testBlockPlaceholder Active

I think I have the Unit test fixed.

Closing a dupe of 🐛 [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing Active

This is being tracked in the meta also: 🌱 [meta] Known intermittent, random, and environment-specific test failures Active

I just ran into this in an issue I was working on.

There were a couple failures:

Layout Builder Blocks (Drupal\Tests\layout_builder\Functional\LayoutBuilderBlocks)
┐
├ Behat\Mink\Exception\ResponseTextException: The text "Placeholder for the "The block label" block" appears in the text of this page, but it should not.
│
│ /builds/issue/drupal-3091671/vendor/behat/mink/src/WebAssert.php:907
│ /builds/issue/drupal-3091671/vendor/behat/mink/src/WebAssert.php:312
│ /builds/issue/drupal-3091671/core/modules/layout_builder/tests/src/Functional/LayoutBuilderBlocksTest.php:219
┴

and

Drupal\Tests\views\Unit\Plugin\argument_validator\EntityTest 0 passes 1s 1 exceptions
FATAL Drupal\Tests\views\Unit\Plugin\argument_validator\EntityTest: test runner returned a non-zero error code (2).
Drupal\Tests\views\Unit\Plugin\argument_validator\EntityTest 0 passes 1 fails

I pushed up a fix that uses the same kind of funky technique as #2853592: Cacheability metadata can't be set from within argument default handlers. → .

I added a fail test that exposes the bug. Now we just need to fix it!

Great! I'm going to change this to a support request and mark it fixed so that we can get credit to @jonathan_hunt.

Cheers!

The other issue is some protective code that's added to Views UI. I think that prevents a WSOD when editing a View in Views UI, and it may prevent saving a misconfigured View.

However, there are no update hooks or the like to fix existing Views configuration that has extra spaces. Nor would that issue fix any extra-space bugs that would be the result of a bad template or something like that.

And it sounds like that's where your problem crept in, right? Trailing spaces in twig (either in the View config or in the template)?

If I'm understanding correctly, then there is no bug in the viewfield module. This module in no way allows configuration like "Output this field as a custom link". The way I see this is that when a misconfigured View is referenced by viewfield there can be trouble. But that's a problem with the View configuration, not a problem with viewfield.

Am I wrong? Should we close this as "Works as Designed"?

I spun up umami with D10.3.8 and I'm not having any luck triggering the error. I made a content type with a viewfield and added a few of those nodes. Then I made a view of those nodes. I used [node:nid] as a argument for one of the viewfields. Still no errors.

While the txt file in #8 is non-unhelpful, it's only so useful since there are many other dependencies and I can't import that config.

I don't think there's anything I can do until there's either:

1. More of a backtrace with the error

or

2. Steps to reproduce starting with a new install

Thanks for the report.

I just ran the tests on current and previous majors and unfortunately did not get any fails.

https://git.drupalcode.org/project/viewfield/-/pipelines/340030

Darn. Some fails would have helped.

I created an upstream issue (I guess?) hoping for a new build script that generates a dist js.

https://github.com/CMSgov/hpt-validator/issues/62

And there it is!

Merged! Nice work, all!

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

danflanagan8 → created an issue.

Back to NW just to get that logo in an MR. Thanks!

I think #11 looks great, @sourojeetpaul.

If you put that in an MR, I'll happily merge it. :)

Hi @sourojeetpaul!

I really like this part of your new logo:

What if we make that the entire logo? That would look great at all sizes. It's also a good representation of the module, right? We're looking into the database and seeing the relationships.

I'm going to set back to NW, but I think we're really close to a good solution now!

MR and patch both look good now!

The patch looks good. I searched the 1.1.x branch for all occurrences of const string and there were exactly 8 of them. All are resolved in the patch.

One of them got missed on the MR though. Back to NW for that.

My team just ran into this. It's not an issue on the 2.x release, luckily.

I think you nailed it, @kristiaanvandeneynde:

I think this shows why we should have more classes in core marked as internal and/or final.

Agreed! I was kind of surprised wasn't internal.

Thanks for your comments to me and your work on the issue. Cheers!

Thanks, @kristiaanvandeneynde.

Am I really allowed to pass four arguments to a constructor that accepts only 3 arguments? We were passing the EntityTypeManager as the fourth argument rather than relying on the already existing BC layer for that fourth argument (lots of work on this class lately!).

I'm still curious about removing a property from the class:

Beyond that, the `$cache` property is removed from PermissionsHashGenerator. Isn't that pretty big BC no-no kind of stuff?

My extension of PermissionsHashGenerator was using that property and therefore completely broke.