🇺🇸United States @jrb

We definitely do not want to just remove that return from content_notify_node_presave() because the code below that section should only run if the node is published. I think the problem is just that the logic in the conditionals that set $criteria is wrong and inconsistent in how it behaves with moderated and unmoderated content.

As noted, if the Workflows module is enabled, it only looks that the moderation state, regardless of whether that content type has moderation enabled (or if that's what is wanted). The current logic also only works if there was a previous status AND that status changed. So, if moderation is used and content is directly published or currently-published content is edited and kept published, it will be ignored. This is different from the logic used when Workflows is not enabled. In that case, it simply looks to see if the content is published.

I've attached a patch that makes the logic consistent, regardless of whether the Workflows module is enabled or not. With this, it checks:

1. Is "Use transition criteria" checked?
2. Is the Workflows module is enabled?
3. Does the content type of the current node actually use moderation?

- If all of those are true, it then checks to see if the current moderation state matches the "To state" setting.
- If any of those are false (e.g. if you don't want to use the transition criteria or moderation isn't used for this content type), then it does not look at moderation state and uses the "Published" status instead.

The patch also updates the description of the "Use transition criteria" setting so it's clear how it works (per above).:

If not checked, the criteria is just that the node being saved is "Published". If checked, the criteria is that the node being saved is in the state specified below.
Note: Content types that do not use a moderation workflow will ignore this setting and use the "Published" status only.

heine → credited jrb → .

Revert update to the steps for changing the href of an existing modal link. The previous steps do actually work, and the problem we were seeing was related to Ajax calls unrelated to changing that link. *sigh 

Update the steps for changing the href of an existing modal link because the old method no longer works in D10.3+.

I just recreated on simplytest.me with the dev version. If you're quick, you can see it here:

https://master-ku0ikjwma4sz1vunwkfcmv7o3hvqcjrt.tugboatqa.com/

The "test" user is active. The two log messages only seem to occur after the first failed login, not with the first (I updated the steps above).

jrb → created an issue.

Patch in #20 re-rolled for 10.3.5.

Re-rolled patch for 3.1.

We were seeing tens of thousands of SQL queries due to this code loading all fields of all entities just to get their labels for the select list. The attached patch uses a custom query to select only the entity ID and label. It resulted in a huge performance improvement for us.

Here's a patch from the MR.

Note that this patch is for 11.x while the patch in #2 above will apply to 10.x.

I think is actually due to a bug in BigPipe and core ajax.js. I created a separate issue here:
🐛 BigPipe error - An error occurred during the execution of the Ajax response: LoadJS Needs review

That new issue includes a patch that fixes the bug.

I think is actually due to a bug in BigPipe and core ajax.js. I created a separate issue here:
🐛 BigPipe error - An error occurred during the execution of the Ajax response: LoadJS Needs review

That new issue includes a patch that fixes the bug.

I think is actually due to a bug in BigPipe and core ajax.js. I created a separate issue here:
🐛 BigPipe error - An error occurred during the execution of the Ajax response: LoadJS Needs review

That new issue includes a patch that fixes the bug.

For those, having this JavaScript error as mentioned by @tonka67 in #89 and others:

An error occurred during the execution of the Ajax response: LoadJS

I don't think that problem is directly related to this and this issue is closed, so I created a separate issue here:
🐛 BigPipe error - An error occurred during the execution of the Ajax response: LoadJS Needs review

That new issue includes a patch that fixes the bug.

Here is a patch that adds a counter and uses that to make sure that uniqueBundleId is always unique.

This fixes our issue with Flag and should fix the other referenced issues above.

jrb → created an issue.

Here's a patch to add an optional "Reset" link to Date Range facets.

jrb → created an issue.

This is not really a problem with this module (devel_kint_extras)-- it's a problem with Devel itself and its implementation of ksm(). However, the Devel module don't use the issue tracker here on d.o. Instead, it uses GitLab.

There's an issue for this problem here that includes two possible solutions:

https://gitlab.com/drupalspoons/devel/-/issues/525#note_1988672373

The patch here is a workaround for this issue: #3456176-45: 10.3 upgrade now missing status-message theme suggestions → .

This looks to be due to this core change: 📌 Use MessagesCommand in BigPipe to remove special casing of the messages placeholder Fixed .

This attached patch makes the Date Range Facet widget work correctly with Ajax. It does not require the patch in #3273136: Selecting date clears other facet values when using AJAX → and should fix that issue as well.

It uses the trigger() method with the facets_filter event, the same way the other Facets widgets do. I believe this is the recommended way to make Facets work with JavaScript / Ajax.

One caveat here is that we needed to call Drupal\Core\Theme\Registry directly because the default "theme.registry" service only returns data for the *active* theme. The core theme registry class called here is marked as @internal, but we could not figure out any other way to do this.

Well... The signature for that class' constructor changed in Drupal 10.3! I've updated the code in the issue fork and added a new patch that will make this work with 10.3 and prior versions.

I've pushed changes to the issue fork. Attached is a patch. This isn't live yet for us, but does work in local testing.

I added some basic documentation to the README:

When you have set up Drupal's standard crontab job, the Modal page scheduling will be processed during each cron run. However, if you would like finer granularity for scheduling but don't want to run Drupal's cron more often, you may use the Modal Page cron handler provided. This is an independent cron job which only runs the scheduler process and does not execute any cron tasks defined by Drupal core or any other modules.

Modal page's cron is at /modal-page/cron/{cron-key}. A sample crontab entry to run the scheduler every minute might look like:

* * * * * wget -q -O /dev/null "https://example.com/modal-page/cron/{cron-key}"

or

* * * * * curl -s -o /dev/null "https://example.com/modal-page/cron/{cron-key}"

The scheduler may also be run with a Drush command:

drush modal_page:cron

jrb → created an issue.

New patch for 4.1.0 that includes the option to show all matches.

For this:

And shouldn't there be any possibility to show the rest of the option, e.g. click on "show all results" or something similar? If not relevant duplicates can be hidden.

That could be nice as a future update, but could lead to UI issues if there were hundreds (or thousands) of matches. Then, we'd probably need to add some sort of paging which would complicate things even more. I'd suggest just adding the new "Number of matches to show" functionality as is for now.

Updated patch for 3.0.12.

In our case, we were seeing the 3 deprecation messages when an anonymous user tried to access a Data Export display of a View that required authentication.

The patch in #32 fixes the issue.

I think this is a duplicate of 🐛 active-link.js throws JS error if query string parameter contains a single quote Needs work

Per that issue, it seems that the actual problem is the ' (single quote) in the queryString variable that causes this line of JavaScript (prior to what the patch here changes) to produce what will be an invalid selector:

      const querySelector = path.currentQuery
        ? `[data-drupal-link-query='${queryString}']`
        : ':not([data-drupal-link-query])';

In that issue, the solution is to escape the single quote. That fixes this issue as well. The use of CSS.escape in the patch of this issue, prevents the errors from being thrown, but I don't think it actually fixes the problem. The resulting selector still won't be valid-- it will just be escaped. I think the solution in that issue is the way to go.

Turning off caching on the Widget and Widget (table) displays of the core Media library View definitely fixes the issue.

The BulkForm plugin in core/modules/views/src/Plugin/views/field/BulkForm.php makes itself not cacheable using UncacheableFieldHandlerTrait, so I tried something similar on MediaLibrarySelectForm per this attached patch. IT DOES NOT WORK, but I wanted to leave it here as a reference.

I assume this doesn't work because of the way the placeholder replacement is done per #3401815-9: Media library widget broken when using exposed filter → .

DO NOT USE THIS!!!!!

Building on #12 above by @asherry, if you paste this into the console it will highlight in red the the items that are wrong and will cause issues:

jQuery('.js-media-library-item').each(function() {
  var $wrapper = jQuery(this);
  var label = $wrapper.find('.form-item__label').text().replace('Select ', '').trim();
  var name = $wrapper.find('.media-library-item__name').text().trim();
  if (label !== name) {
    $wrapper.attr('style', 'background-color: red;');
    console.log('Item displayed as ' + name + ' has incorrect label and value.');
  }
})

Yes, it is the same.

*sigh. I searched but didn't see it. Particularly this comment referencing the value in the checkbox being wrong: #3401815-8: Media library widget broken when using exposed filter →

Was going to comment there with some code changes I just tried.

jrb → created an issue.

I've created a sandbox module that does this here:

https://www.drupal.org/sandbox/jrb/3433126 →

In order for this to work, the Google Tag Manager gtag script must be included on the page by some other method. This can be done with a custom script or with a module like Google Tag → .

Just enable this module; and, when the user consents to cookies, it will call gtag with this:

gtag('consent', 'update', {
  'ad_user_data': 'granted',
  'ad_personalization': 'granted',
  'ad_storage': 'granted',
  'analytics_storage': 'granted'
});

Depending on what the maintainers of this module would like to do, this code could be folded into this module as a submodule or I could release it as a full module. Either way is OK with me.

jrb → created an issue.

This assert() was removed in 📌 Leaf lifetime exceeds 60s in large migrations Fixed , so this is no longer an issue in any branch (1.x, 2.x, and 3.x).

Just FYI, the proposed change won't fix this issue in layouts created by the Layout builder library → module (used on 14K sites). The structure of its configurations is different from that of Layout Builder, so it wouldn't be simple to just add in support for updating them. For our site, we ended up just manually editing the config.

Who knows how many other modules may be supporting fields that could possibly use Smart Trim, so I don't know if it's realistic for you to support every use case. Unless there were some other way for you to find where Smart Trim was used? We grepped the config directory, but that wouldn't help with automatically fixing the config.

Maybe it would make sense to just output a warning message that says something like this?

If you're using Smart Trim in places other than entity view displays, you may need to manually update its configuration.

Here's an update to the patch in #4 with the code that adds the event listener wrapped in a once() call so it doesn't get added multiple times (per the suggestion in #9).

The changes in MR 388 fixed the issue for us. Attached is a patch with the diffs as of today.

Added explanation of how to change the href of an existing modal link.

I tried this MR with Drupal 10.1.6 and ran into two issues with a text format where the "Limit allowed HTML tags and correct faulty HTML" filter is enabled.

1. When I first add the new "Div" button, it throws this error:

The following tag(s) are already supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: Div Manager (<div>).

If I remove <div> from the "Manually editable HTML tags" field, the error goes away.

2. When trying to save, I get this error:

The current CKEditor 5 build requires the following elements and attributes:
<br> <p class="iframe-responsive messages messages--status messages--warning messages--error messages--blue messages--purple messages--gray text-align-left text-align-center text-align-right text-align-justify"> <h2 id class="text-align-left text-align-center text-align-right text-align-justify"> <h3 id class="text-align-left text-align-center text-align-right text-align-justify"> <h4 id class="text-align-left text-align-center text-align-right text-align-justify"> <h5 id class="text-align-left text-align-center text-align-right text-align-justify"> <h6 id class="text-align-left text-align-center text-align-right text-align-justify"> <a class="button" hreflang href id target="_blank" data-entity-type data-entity-uuid data-entity-substitution> <* dir="ltr rtl" lang> <cite> <dl> <dt> <dd> <blockquote cite> <ul type> <ol start type> <strong> <em> <s> <sub> <sup> <li> <hr> <table> <tr> <td rowspan colspan> <th rowspan colspan> <thead> <tbody> <tfoot> <caption> <drupal-media data-entity-type data-entity-uuid alt data-caption data-align> <div class id style title>
The following elements are missing:
<div style>

As you can see, <div style> is actually on the list as <div class id style title> (at the end). I can manually edit the config file and import to get it to work, but then I still can't save it via the UI.

So, as it is now, we can't use this with the "Limit allowed HTML tags and correct faulty HTML" filter enabled.

Looking at the code in image.module, it calls $style->supportsUri($variables['uri']) to see if the image type is supported. If not, it just returns the original image then logs the warning. Given that the image is displaying, I don't think there's anything this module can do about it.

Work on this warning was in #2652138: Image styles do not play nicely with SVGs → . Would be nice to suppress this warning so it doesn't fill up the logs, but I don't think that's this module's problem.

We're seeing this, too. In our case, we've got:

1. A Paragraph type that includes an entity reference field to a Media entity (image).
2. The image field on the Media entity allows the following extensions: png, gif, jpg, jpeg, svg
3. The display of the Paragraph type uses the "Thumbnail" format with an image style.

When a node that includes an instance of this Paragraph type is viewed, the SVG is correctly shown, but we get a warning like this in the logs:

Could not apply XXX image style to public://my-icon.svg because the style does not support it.

Heh. Just noticed that this issue is 6+ years old!

@RohitRawat676

While it is nice to make the code follow coding standards, it's not good practice to change code unrelated to the issue at hand in an issue patch. In this case, the previous patch changed 3 lines, but your patch changes over 150 lines. This makes it much more difficult to test that the patch fixes the issue without introducing any other problems.

If you would like to suggest changes to make the code of this module (or any module) follow Drupal coding standards, that would be welcome, but it would be best to create a separate, new issue and add your patch there.

We're also working on improving the editorial experience here and tried out the MR in #22. It sorta worked for us, but we made a few changes:

1. In our case, we needed other templates added to the edit page, not just the Paragraph templates. For example, one of our Paragraph types displayed a list of node teasers, but those teasers were displayed using the default node template rather than the one defined in the default site theme. To solve this issue, we added a new "Additional templates to load" setting to the "Default theme" form. Any templates set there will also get added.

2. To support the new setting field above, we needed to load the theme registry for the default theme. We added a helper function to do this. Having that default theme registry array allowed us to set values like this:

$theme_registry[$template_name] = $default_theme_registry[$template_name];

This also allowed us to simplify the logic used to add Paragraph templates in the same way. Now, rather than looking at the file system to see what templates exist, we can look directly at the theme registry.

One caveat here is that we needed to call Drupal\Core\Theme\Registry directly because the default "theme.registry" service only returns data for the *active* theme. The core theme registry class called here is marked as @internal, but we could not figure out any other way to do this.

3. The MR in #22 will only add the Paragraph templates to the admin theme if you flush the cache while on the node add/edit page due to this code:

$route_name = \Drupal::routeMatch()->getRouteName();
$route_is_admin = \Drupal::service('router.admin_context')->isAdminRoute();

// Just do that if the theme is 'admin' and the page is the add or edit node form.
if($route_is_admin && ($route_name == 'entity.node.edit_form' || $route_name == 'node.add')) {

The issue is that hook_theme_registry_alter() functions are run only when the cache is flushed, not on every page load. So, for this to work, the conditional above must be removed. That will mean that the Paragraph templates are always added to the admin theme which is probably OK. That is how the MR worked in the first place when the cache was flushed from the node add/edit page, but I don't think that will cause any problems.

I've updated the branch in Git and added a patch.

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

If you just want to use this module, there's no need to apply this patch.

We'd like to use this module with the JSON:API Search API module, but are seeing the same problem.

Wouldn't it be enough to just check if \Drupal\jsonapi\Routing::isJsonApiRequest(...) is TRUE?

Jsonapi_menu_items also implements the requirements for isJsonApiRequest to return TRUE.

That does not look like it would work. That Routes::isJsonApiRequest() method basically checks this:

str_starts_with($defaults[RouteObjectInterface::CONTROLLER_NAME], static::CONTROLLER_SERVICE_NAME)

So, it's looking to see if the "_controller" defaults setting starts with "jsonapi.entity_resource". That's not the case for the JSON:API Menu Items module (or the JSON:API Search API).

Here's what the the route defaults look like for core and the other two modules.

Core - Taxonomy terms for a vocabulary

Route: jsonapi.taxonomy_term--keywords.collection
Array (
 [_controller] => jsonapi.entity_resource:getCollection
 [resource_type] => taxonomy_term--keywords
 [_is_jsonapi] => 1
)

JSON:API Search API - jsonapi_search_api

Route: jsonapi_search_api.index_example_index
Array (
 [_jsonapi_resource] => Drupal\jsonapi_search_api\Resource\IndexResource
 [_jsonapi_resource_types] => Array (
  [0] => node--page
 )
 [index] => a20ddf02-ab33-4ab4-a0bb-67abc14be25f
 [resource_type] => search_api_index--search_api_index
 [_is_jsonapi] => 1
)

JSON:API Menu Items - jsonapi_menu_items

Route: jsonapi_menu_items.menu
Array (
 [_jsonapi_resource] => Drupal\jsonapi_menu_items\Resource\MenuItemsResource
 [_is_jsonapi] => 1
)

In each case, _is_jsonapi is set to TRUE, so it looks like that would be the best thing to check. I've attached a patch that adds a check for this to the existing conditional in ResponseSubscriber::onResponse(), but it may be possible to just replace the 3 checks with this single check instead:

    if ($this->routeMatch->getRouteObject()->getDefault('_is_jsonapi')) {

That would work for core and these 2 modules.

I've updated the title and description for this issue to include the JSON:API Search API module (and potentially other modules).

Attached is a patch of the MR as of today for use in Composer.

In Drupal 10.1.4, the patch in #8 did not work for us. In js/facets-views-ajax.js, options.url did not include the facets query string variables, so they were not getting sent in the URL that Ajax uses. I'm not sure if we're seeing something different from the people that #8 worked for or if maybe something changed in D10?

In our case, our facet was using the variable opp and our exposed form had the variables keys and open. The URL that Ajax was using when facets were active was this (formatted for clarity):

/views/ajax
?_wrapper_format=drupal_ajax
&keys=test
&open=0
&view_name=example_search
&view_display_id=block_1
&view_args=
&view_path=%2Fsearch-results
&view_base_path=search-results
&view_dom_id=c31d02539fca221547dc141fdacf4969d8c0193bd1c980cd824405906d9adf32
&pager_element=0
&_drupal_ajax=1
&ajax_page_state%5Btheme%5D=example
&ajax_page_state%5Btheme_token%5D=
&ajax_page_state%5Blibraries%5D=admin_toolbar%2Ftoolbar...

You can see that the URL there has keys and open, but is missing the opp variable which was in the URL as &opp%5B0%5D=discipline%3A255&opp%5B1%5D=eligibility%3A414 at this time.

We were able to fix this by adding code to Drupal.Ajax.prototype.beforeSend() that looks at the query string parameters in options.url, compares them those to the ones in the current URL, and adds any that are missing.

That's what this patch does.

Changed date formats to be consistent. Added date to heading.

For more clarity, added actual date for D9 EOL rather than just "November 2023".
Date from here:  https://www.drupal.org/project/drupal/releases/9.5.11 →

@bajah1701

I looked into the code a bit and noticed that line 238 & 255 is making use of the $identifier that isn't defined.

$identifier is defined if $exposed is set, and it will be in lines 238 and 255 (it's checked).

@DieterHolvoet

I don't think these changes to $user_input are even really doing anything: $user_input = $form_state->getUserInput(); is not storing that array as reference and $form_state->setUserInput($user_input); is not called after doing those changes.

This is definitely true, but was an issue in the code prior to this patch being added. Looking waaaaaay back, it actually appears to have been broken in this commit from 9 years ago that changed $form_state from an array to an object!

https://git.drupalcode.org/issue/drupal-3330100/-/commit/de5fe262f2c2371...

In it, they replaced all direct uses of $form_state['input'] with code like $user_input = $form_state->getUserInput() . They used $form_state->setUserInput($user_input) afterwards most everywhere, but not here. Maybe it's not all that important since no one has noticed in 9 years, but I'd say it should be added back.

*****

I don't have time right now to update the issue fork, but attached is a patch that adds $form_state->setUserInput($user_input) to the changes from the current MR. This fixes an issue for us where having "&price=" in an URL where price should be an array causes a the fatal error.

Here's a patch using the second method from above (don't try to get the session if it's a Drush request).

jrb → created an issue.

In Drupal 8+, it seems like Facets (at least the basic ones) just work with Ajax if you enable it on the View. Maybe a new version of this module isn't needed?

We've found some issues with the range slider and a contributed module for date ranges, but links and checkboxes do work.

Here's the patch in #135 re-rolled for 10.1.

Here's a patch to get things started. This will:

1. Add an optional parameter to findTables() to exclude SQL views from the query.
2. Use that parameter in the mysql_requirements() call in mysql.install.

jrb → created an issue.

Attached patch makes it show the original number of items after the search is reset.

jrb → created an issue.

We were seeing something similar, too, where Facet blocks were not showing up sometimes with BigPipe enabled. It was not a problem on every page load, though. Seems like it might be due to a race condition?

Just FYI, you can disable BigPipe for Facet blocks by adding this to a custom module:

use Drupal\Core\Block\BlockPluginInterface;

/**
 * Implements hook_block_build_BASE_BLOCK_ID_alter().
 */
function example_block_build_facet_block_alter(array &$build, BlockPluginInterface $block) {
  // Disable BigPipe placeholder for this block.
  $build['#create_placeholder'] = FALSE;
}

Patch in #4 re-rolled for 2.0.6.

D'oh! I misread the error in the test results the actual issue was that the patch in #5 that added the schema file had check_filesystem instead of check_filesystems. This latest patch fixes that.

Let's see if this passes now!

These patches have been failing testing, but not due to anything specific to the patches themselves. The issue seems to be with the tests for the module itself:

Warning: Your XML configuration validates against a deprecated schema.

This new patch addresses the code-standards messages also reported by testing:

health_check.module ✗ 6 more
line 14 Hook implementations should not duplicate @param documentation
14 Missing parameter type
39 Expected newline after closing brace
71 Inline @var declarations should use the /** */ delimiters
100 Expected newline after closing brace
102 The array declaration extends to column 111 (the limit is 80). The array content should be split up over multiple lines

src/Form/HealthCheckConfig.php ✗ 1 more
11 The class short comment should describe what the class does and not simply repeat the class name

If someone else could try out this latest patch with the try/catch added, it should be RTBC and a big improvement.

Here is an updated patch that just wraps each of the cache and file system tests in a try/catch. This makes it behave much more nicely if one of the tests actually fails. We've been using this in production on multiple sites for over 6 months.

The interdiff is ignoring whitespace.

In cleaning up my sandbox modules, I saw this issue created for the sandbox module → that became the D8 version of this module → .

Moving it to the real module and closing it as outdated. If it's still an issue with the real version, please re-open.

This is now a full project. This change was committed to the D7 branch.

There's now a D10 version of this module.

This is now a full project. This change was committed to the D7 branch.

There's now a D10 version of this module.

Thanks, yogeshsevak & Bushra Shaikh!

Version 2.1.0 is compatible with Drupal 10.

Yes, this is still an issue.

The patch in #19 still applies correctly to the 2.0 branch and adds the requested feature. The comment in #14 describes how to test this, and the patch does include a test.

We've been using this patch in production for 3+ years.

The patch in #16 did not work for us. We got this error with each project that it checked:

Message TypeError: Argument 1 passed to Drupal\upgrade_status\TwigRecursiveIterator::__construct() must be an instance of Drupal\Core\Extension\Extension, string given, called in /app/web/modules/contrib/upgrade_status/src/TwigDeprecationAnalyzer.php on line 62 in Drupal\upgrade_status\TwigRecursiveIterator->__construct() (line 20 of /app/web/modules/contrib/upgrade_status/src/TwigRecursiveIterator.php)

This new patch fixes that error in TwigDeprecationAnalyzer and LibraryDeprecationAnalyzer.

Patch in #4 re-rolled for latest.

Related:

There is a core issue suggesting that optional parameters should be added to Json::encode($menuConfig) to allow passing of flags:

📌 Json::encode() and Json::decode() limit what we can do with JSON Needs work

If that patch does land, this code could be updated. Until then, this patch uses the suggested workaround of just calling json_encode() directly so we can pass our own flags.

Attached patch formats the JSON in the config using JSON_PRETTY_PRINT.

Note that, before the patch, the TB Mega Menu code called Json::encode($menuConfig) which made this call:

public static function encode($variable) {
  // Encode <, >, ', &, and ".
  return json_encode($variable, JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT);
}

The patched code now just calls json_encode() directly with those same options plus JSON_PRETTY_PRINT.

jrb → created an issue.

Patch re-rolled for latest.

Patch re-rolled for latest.