@codebymikey

ConfigEntityUpdater currently leads to settings such as "processor_settings" to be reordered by weight rather than alphabetically as per the schema.

This is because ConfigEntityUpdater flags the entity has having trustedData(), meaning that it should no longer do any side effects such as removing/adding processors.

I added the explicit sorts as a fallback in case it's decided that the module should still attempt those kind of configuration changes.

codebymikey → changed the visibility of the branch 3454657-dont-rely-on-bundles to hidden.

Added change to \Drupal\pathauto\Entity\PathautoPattern::addSelectionCondition() which persists the UUID if the configuration includes a uuid property saving the need to regenerate a unique UUID everytime.

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

codebymikey → created an issue.

I just thought it made more sense to follow the common convention of having the configuration updated as part of the module update as necessary just like core did with the system_post_update_sort_all_config() hook 📌 Config export key order is not predictable, use config schema to order keys for maps Fixed , since that's one of the main reasons the ConfigEntityUpdater exists.

This ensures the config changes are a lot more predictable and consistent if it's done once as part of the module update, so subsequent config exports will only include things the user actually changed in the index.

the current unreliable ordering just means that there might be nonexistent changes reported for exported configs.

I think a change in the dependency order is an existent change (just like a change in config key orders according to the schema would be).

The update hook was useful for my use case because one of my distributions has multiple search index entities included and their dependencies needed to be automatically reordered in order to match what they should be on a clean install, otherwise they'd show up in my config diffs as a missing distribution change.

Either way, I don't have that strong a stance on it either way if you feel it's detrimental to the DX, and can reapply the post update hook myself, but I personally find it easier if such config changes are automatically documented for the developer via the update hook, so they're aware and can choose what to do with it after the database update (keep, or ignore it)

codebymikey → created an issue.

I was working on a workaround for this before I stumbled upon this issue, and have subsequently pushed into an issue fork for reference.

Mine was caused because I had multiple custom modules which handled different functionality. And the "base" module had the search api index as well as the base field storage entities, but not the individual bundles (since those would be handled when the optional modules were installed later down the step).

Given that the issue only affected site installs for me, I added a condition to only load the base field storage properties during that state. However if it makes sense to remove that condition, then that's perfectly fine too.

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

codebymikey → created an issue.

For those kind of plugins which already make calls to certain Wordpress PHP functions/filters for registering.

We can potentially leverage the same approach as the D7 to D10 Retrofit initiative, implementing it in a gutenberg_wordpress_retrofit contrib or submodule which'll provide compatibility layers for existing WordPress plugins without requiring a port of the API.

codebymikey → created an issue.

codebymikey → created an issue.

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

Implemented feedback from #4

codebymikey → changed the visibility of the branch 3230130- to active.

codebymikey → changed the visibility of the branch 3230130- to hidden.

Yes, this is replicable on a clean installation using Simplytest. And sorry I mispoke, the behaviour more apparent when the "Display title" option is ticked on the block.

Relevant links available here (username: admin, password: admin):

• Text field with "default value"
• Layout Builder with the new "Text with default value" field and a Title set
• Sample content with the bug

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

codebymikey → created an issue.

codebymikey → created an issue.

codebymikey → created an issue.

codebymikey → changed the visibility of the branch 3418353-restrict-who-can to hidden.

Following on from the feedback from #55, updated the logic so that only the relevant progress elements were removed as a side-effect.

The previous change inadvertently broke integration with Gutenberg 2.8 when editing Media blocks, because it removed the progress bar that already exists in the React lifecycle before it should've causing it to crash.

marcofernandes → credited codebymikey → .

Should be fixed in 🐛 Assets are not served over S3 RTBC

I believe the $block variable/argument is missing, which means certain functionality wouldn't work as expected.

https://git.drupalcode.org/project/gutenberg/-/merge_requests/129/diffs#...

+    $is_not_default = isset($block['attrs']['layout']['type']) && $block['attrs']['layout']['type'] !== 'default';
+    if ($is_inner_container || $is_not_default) {
+      return $block_content;
+    }

And will need to reapplied to 3.x as well

marcofernandes → credited codebymikey → .

This functionality is supported in the https://www.drupal.org/project/search_api_exclude_entity → module. So might be worth using that over this module.

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

Since the previous queue wasn't working properly, you might be in a situation where the next queue is scheduled to run in 7 days time, which means that a manual reset might be required in order to process them as soon as possible:

drush php-eval '\Drupal::state()->set("smart_date_recur_check", 0);'

Given how fragile the reimplemention of parse_str could possibly be, what are the thoughts on introducing an additional dependency like league/uri-query-parser or league/uri-components to handle it?

Since they probably have more test coverages and well-tested.

We could also try using something along the lines of https://www.php.net/manual/en/function.parse-str.php#126789 as a base as well.

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

I think the points raised in #15 regarding the regex only working if the paramter is the first entry are still relevant.

I'm not sure this or the original behaviour works as expected when the query parameter is not the first entry, or the first query ends with a q or render.

I've provided some example test cases to showcase the behaviour here: https://playcode.io/1830989

const queryStrings = [
  'q=/path',
  'faq=should_not_be_affected',
  'myrender=should_also_not_be_affected',
  'q=/path&a=1&q=/second-path',
  'q=/path&render=value',
  'q=/path1&q=/path1&faq=true&dont_render=1&q=/path&render=render_value&page=1&final_value=1',
];
for (const queryString of queryStrings) {
  const output = [];
  output.push(queryString.replace(/q=[^&]+&?|&?render=[^&]+/, ''));
  output.push(queryString.replace(/q=[^&]+&?|page=[^&]+&?|&?render=[^&]+/, ''));
  output.push(queryString.replace(/(^|&)(q|render|page)=[^&]+/g, '').replace(/^&/, ''))
  console.log(output);
}

This might be potentially supeceded by 🐛 Query string is appended multiple time after each AJAX request Needs work which now attempts to use a URLSearchParams object to remove those same values. But we may also look into using the updated regex provided instead, and leave that as a follow-up.

I also believe the render query parameter might be legacy code and no longer relevant in D9+, so can probably be removed since I can't find any references to it in core.

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

Probably a duplicate of 🐛 Media Library views fail to reset pager when filters are applied Needs work

Appreciate the excellent work done here! The code also makes a lot of sense - following the implementation of #3278415: Remove usages of the JavaScript ES6 build step, the build step itself, and associated dev dependencies → , the browser shim is probably no longer necessary.

I'd ideally want to continue work off your existing issue, I believe the work is better off consolidated in 🐛 Query string is appended multiple time after each AJAX request Needs work since it's been around for longer, and tackles a similar issue. I'll try and port your solution over to that issue, and we can probably close this as a duplicate.

Updated the issue summary to state the cause of the issue.

This appears to be a duplicate of 🐛 Media library modal field widget does not render selection checkbox after search Active , and seems to have been resolved in 10.3.x and 11.x

I do not get why we have this test case:

I believe this might be a non-issue, as I included the test case directly from the HTML Standard page mainly to test that CKEditor or our HtmlBuilder doesn't mangle up the existing markup if that's what the original source code is.

I believe browser quirks like these should be left for the content editor to handle themselves rather than us converting it for them, especially if it might involve changing string values like:

const example = 'Consider this string: <!-- <script>';

to:

const example = 'Consider this string: \x3C!-- \x3Cscript>';

It can lead to unintentional bugs.

And with regards to:

And that is transformed into:

<script>
const example = 'Consider this string: &amp;lt;!-- &amp;lt;script&amp;gt;';
console.log(example);
&amp;lt;/script&amp;gt;
&amp;lt;/body&amp;gt;</script>

Is it actually being double encoded to HTML in your tests, or is the D.O formatter in the issue encoding it for you? As my local tests doesn't involve any encoded HTML entities as that's what 🐛 JavaScript operators in Needs work was meant to mitigate in the first place.

Nonetheless, I've added an updated test-case to showcase the behaviour that'll also work for test-only pipelines even though we are no longer using a @dataProvider.

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

@Wim Leers

Why is that not a problem? 🤔 I feel like I'm overlooking something? 🙈 Sorry!

I introduced 3 new tests in the latest commits (which weren't part of the original test-only pipeline that was ran), which is probably the main discrepancy.

The https://git.drupalcode.org/issue/drupal-3364884/-/jobs/1119730 job was for the 490e9434 commit which still had 3 test cases not 6.

@acbramley sorry about the delay responding to this

@codebymikey so you went from <8.8 to 10.3 in a single update?

No, I just meant that the original installation of the site started before 8.8, so the update hook that was ran at the time that Taxonomy terms were made revisionable didn't do any of content population stuff, and is why this bug probably occurs for it.

Implemented the feedback, however it makes the test only tests a little harder to test for.

Updated issue summary.

Updated the logic so that all the duplicated logic is handled by a single function, as well as removed all the disable logic from hook_page_attachments() to hook_page_attachments_alter() so that it can detect dynamic libraries such as the google_tag one that might be added.

Provided a MR which implements the fix in an event subscriber rather than through hooks.

codebymikey → changed the visibility of the branch 3364884-javascript-operators-in to hidden.

@Wim Leers good spot on the test-only changes! I switched to the @dataProvider last minute and didn't really contemplate the test cases properly.

Those relevant features have been implemented, and awaiting review

Added a MR with the relevant test, and requires review.

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

Seems related to 🐛 seven.jquery.ui css assets may be added the page multiple times, breaking dialog button on Drupal 10.1 Active

codebymikey → created an issue.

I ran into this issue myself with the google_tag module.

It seems to be because google_tag's hook_page_attachments() implementation is ran after eu_cookie_compliance's.

This means that the disabled_javascripts check doesn't necessarily work as expected.

I notice that we also implement eu_cookie_compliance_page_attachments_alter() but don't use that to manipulate the attached library definitions. Is there any particular reason we don't handle the disabling of all assets in the alter hook?

It might also be worth implementing a hook_module_implements_alter() to ensure that the module acts on the attachments last.

Thoughts are more than welcome!

Giving the severity of the bug, I believe this patch should probably be added to 3.7.x or 3.7.x be deprecated.

Sorry it took a while to get back to this. I like the approach, but it seems like \Drupal::service('config.installer')->installDefaultConfig('module', 'gutenberg') will override any local configuration overrides the sites might have, so we need special handling for it.

I've pushed an update to the branch with the updated logic, feel free to test and review, and let me know if it all makes sense.

Looks good to me, but a couple feedback:

 dependencies:
+  enforced:
+    module:
+      - gutenberg
+  module:
+    - gutenberg

I don't think the module dependency is necessary, as it'll probably be deleted once the dependencies are recalculated the next time the config is resaved.

I also believe according to the config schema order, the enforced key needs to be the last dependency entry:
Reference:

• https://git.drupalcode.org/project/drupal/-/blob/9c1e1035/core/lib/Drupa...
• https://git.drupalcode.org/project/drupal/-/blob/9c1e1035/core/config/sc...

Attach static patches.

I think you're right @manarth, I just attempted to incorporate the original approach suggested in the original @ThirstySix MR since I thought it was solving a different variation of the issue.

Using the 64bit "J" version lead to broken pipe issues:

Notice: stream_copy_to_stream(): Send of 424 bytes failed with errno=32 Broken pipe in Drupal\clamav\Scanner\DaemonTCPIP->scan()

Notice: fwrite(): Send of 8 bytes failed with errno=32 Broken pipe in Drupal\clamav\Scanner\DaemonTCPIP->scan()

And I've swiftly updated the MR as appropriate.

Added a rebased patch targetting 6.0.x.

And sorry for the lack of update on this, but haven't had the time to implement the feedback.

@acbramley, I believe this might be related to an existing issue 🐛 New revision fields don't have a default value after making an entity revisionable Active , additional documentation on this is available here → .

And because the site I ran this particular update on is fairly old, it was probably before taxonomy terms were even revisionable (<8.8.0), so it was missing the relevant update hook which ensures that the relevant revision fields are prepopulated as part of the revisions update.

So in terms of recreation, I'd say the site needs to have taxonomies created from taxonomies were revisionable. Update to the latest version of Drupal, then have this update applied on top of it.

So it's probably a bit of an edge case, but probably worth being aware of nonetheless.

I applied this patch on 10.x, and upon viewing revisions of existing taxonomy term, it crashes with the following:

The website encountered an unexpected error. Please try again later.

InvalidArgumentException: The timestamp must be numeric. in Drupal\Component\Datetime\DateTimePlus::createFromTimestamp() (line 201 of core/lib/Drupal/Component/Datetime/DateTimePlus.php).
Drupal\Core\Datetime\DateFormatter->format(NULL, 'short', '') (Line: 148)
Drupal\Core\Entity\Controller\VersionHistoryController->getRevisionDescription(Object) (Line: 279)
Drupal\Core\Entity\Controller\VersionHistoryController->buildRow(Object) (Line: 252)
Drupal\Core\Entity\Controller\VersionHistoryController->revisionOverview(Object) (Line: 76)
Drupal\Core\Entity\Controller\VersionHistoryController->__invoke(Object)
call_user_func_array(Object, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 592)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Object, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

And not sure if an update hook is necessary to generate the default getRevisionCreationTime() value, so that it no longer resolves to NULL.

I'm aware this was mainly for 11.x, just thought I'd document in case an additional upgrade path is necessary.

Added a MR implementing the feature.

codebymikey → created an issue.

Updated the MR incorporating @ifcarrionc_globant's latest fix, adding support for unix sockets as well.

Reading the filesize from the resource seems like the most stable way to address the issue.

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

This behaviour can also be overridden for specific routes in a custom module using the following:

/**
 * Implements hook_menu_get_item_alter().
 */
function custom_module_menu_get_item_alter(&$router_item, $path, $original_map) {
  $router_path = $router_item['path'];

  if ($router_path === 'node/%' || $router_path === 'taxonomy/term/%') {
    if ($router_item['number_parts'] < count($original_map)) {
      $router_item['page_callback_original'] = $router_item['page_callback'];
      // Show a 404 response.
      $router_item['page_callback'] = 'custom_module_menu_not_found_callback';
    }
  }
}

/**
 * Callback to flag the response as not found.
 */
function custom_module_menu_not_found_callback() {
  // drupal_not_found(); drupal_exit();
  return MENU_NOT_FOUND;
}

It could be theoretically made into a contrib module which allows users to set the max amount of URL components that are allowed for a specific menu route.

The DatabaseLogger logger already implements the PagerSelectExtender class, so needs to be excluded.

Ideally each logger should implement an interface which states whether they support pagination or not, but in order to keep the feature patch lightweight and avoid any conflicts, it's been implemented this way.

The DatabaseLogger logger already implements the PagerSelectExtender class, so needs to be excluded.

Ideally each logger should implement an interface which states whether they support pagination or not, but in order to keep the feature patch lightweight and avoid any conflicts, it's been implemented this way.

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

codebymikey → created an issue.

A MR and patch for core targetting D10.1 is available here 📌 Make CKEditor5ImageController reuse FileUploadHandler Active .

codebymikey → changed the visibility of the branch 3388985-make-ckeditor5imagecontroller-reuse-10.1.x to hidden.

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

This appears to be a core issue, and should probably be patched in there.

Also:

+    if (!is_resource($file_handler)) {
+      // @see Drupal\ckeditor5\Controller\CKEditor5ImageController
+      $upload = \Drupal::request()->files->get('upload');
+      $file_handler = fopen($upload->getRealPath(), 'r');
+    }
+

Seems like it'd throw an exception if $upload doesn't resolve to an object.

This is a probable duplicate of 🐛 Support image upload plugin for CKEditor5 Active

Attached a version of the patch which makes use of a post update hook to apply the relevant change, whilst avoiding changing the logout_office value if it's already been set.

The attached patch only works when applied on top of https://www.drupal.org/project/o365/issues/3295745#comment-15441452 📌 Make it possible to use multiple Office 365 connectors (5.x) RTBC

Attached a version which uses a post update hook to avoid collision with the existing 🐛 logout Fixed update hook.

Attached a static patch of the current MR supporting the latest 5.0.4 version.

Initially thought of adding a wrapper around the code which returns the cacheable responses using a helper function like this:

/**
 * Returns a trusted response.
 *
 * Taking into account any bubbleable metadata generated.
 *
 * @param callable $callable
 *   The callable to resolve the relevant response.
 *
 * @return \Drupal\Core\Cache\CacheableResponseInterface|mixed
 *   The response.
 */
protected function wrapResponse(callable $callable) {
  $render_context = new RenderContext();
  $cacheable_metadata = new BubbleableMetadata();
  $response = $this->renderer->executeInRenderContext($render_context, $callable);
  if ($response) {
    if (!$render_context->isEmpty()) {
      $metadata = $render_context->pop();
      $cacheable_metadata->addCacheableDependency($metadata);
    }
    if ($response instanceof CacheableResponseInterface) {
      $response->addCacheableDependency($cacheable_metadata);
    }
    elseif (is_array($response)) {
      BubbleableMetadata::createFromRenderArray($response)
        ->merge($cacheable_metadata)
        ->applyTo($response);
    }
  }

  return $response;
}

But it's probably not necessary, and makes the code a bit more ugly to maintain, so opted for stopping the specific metadata we're aware of from leaking.

I added a 3420422-on-3295745-redirect-support branch for supporting redirects in a way that's compatible with 📌 Make it possible to use multiple Office 365 connectors (5.x) RTBC , as well as a static patch that's compatible with #18 📌 Make it possible to use multiple Office 365 connectors (5.x) RTBC (3.0.25)

codebymikey → created an issue.

Added a relevant comment to the commit of the original PR here, but it's easy to miss, so adding as a comment here.

Can't see the specific code that breaks in the pipelines, but I believe this might be breaking due to the way the gutenberg_config_schema_info_alter() dynamically picks up schema fields at the moment (which isn't ideal).

At the point that $config_editable->set($key, $allowed_blocks)->save(); is called, Drupal attempts to validate the config according to the schema, but fails because the schema is still based off the "previous" config value before the media entity was enabled.

I believe since this is an update hook, switching to:

$config_editable->set($key, $allowed_blocks)->save(TRUE);

might yield better results since that won't trigger any config schema validation since we trust the data we pass into the updated config at that point.

But this begs a bigger question as to whether this crash due to new fields only happens under test environments, or will occur if you attempt to add Gutenberg support for new entity types on live sites (I don't believe it'll/should, but might need to explore further).

Attached a static patch for the latest version.

My bad @dqd, it was an oversight on my part, as I assumed @sorlov also created the issue and submitted the patch.

I've retroactively given you credit for this issue.