🇳🇱Netherlands @seanB

Merged! Thanks.

I guess that would make sense when the imagecache external module is installed. I would definitely accept a PR to implement this.

I just didn't have the time to implement this yet, but I agree that it would be a nice feature!

Instead of the URL I think we only need an ID. That being said, we should probably not throw and error. We might be able to do the following:

• Harden the code to show better error messages when something goes wrong
• Automatically extract an ID from URLs before we call the API

Would you be able to create a PR for this?

I think you can technically already use ENV variables or external services to fetch values for the settings.
$settings['media.external_provider.unsplash.access_key'] = getenv('UNSPLASH_ACCESS_KEY');

I'm not opposed to adding support for the key module. Does the key module allows overrides for settings as well, or only for configuration?

Patch is attached. Also created a MR. Please review!

seanB → created an issue.

+1 for moving the removal of the library to a followup!

MR!8515 does not solve the issue. If a node or revision is deleted during the batch process (by something or someone outside of the batch process), $node is NULL. This throws an error when trying to call _node_mass_update_helper.

Since I needed something that works in a current project, fixed the issues found in my last review. I agree we should get answers for the questions in #93.

+++ b/core/modules/user/src/Entity/Handler/BatchCancellationHandler.php
@@ -0,0 +1,216 @@
+      $this->updateEntity($entity, $method);

As found in 🐛 _node_mass_update_batch_process fails during user cancel when revision is deleted Needs review the entity could technically be removed between creating the batch and the execution of this method. We should check if $entity is properly loaded.

+++ b/core/modules/user/src/Entity/Handler/DefaultCancellationHandler.php
@@ -0,0 +1,185 @@
+    $this->storage->save($entity);

As found in 🐛 Revision user incorrectly appears as anonymous user when node author is cancelled Needs work a new revision is created when content moderation is used. We should probably try to prevent this by checking for SynchronizableInterface and doing something like this:

  $original_syncing = $entity->isSyncing();
  $entity->setSyncing(TRUE);
  $this->storage->save($entity);
  $entity->setSyncing($original_syncing);

Created a PR.

seanB → created an issue.

Entity Browser has entity_browser.view.js that performs some magic. Since we do not properly use the Entity Browser modal it seems parts of it do not work as expected. I agree hook_views_pre_render is a workaround for this, but it seems a bit easier that properly integrating Entity Browser modals.

Jean-Paul Vosmeer → credited seanB → .

Created an MR that passed the parameters. Also added a custom ckeditor5 parameter so we can check when to forward it for views.

seanB → created an issue.

+1 from me!

Merged! Thanks again :)

Fixed in 🐛 There is no display available - while referencing, if the view name contains an opening bracket Fixed

Fixed in 🐛 There is no display available - while referencing, if the view name contains an opening bracket Fixed

Merged! Thanks, the tests really help already!

Merged! Thanks all!

I think the conflict project was added to the composer.json file because the project has been renamed from drupal/viewsreferennce to drupal/viewsreference. Which means you should never have both in your composer.json file. So this is probably not a typo (maybe the other maintainers can confirm this?).

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

Merged! Thanks.

Removed all the Drupal CI scheduling.

The change in 📌 Compress ajax_page_state Fixed only affects the ajax_page_state parameter. We should apply the same solution to the viewsreference parameter we are adding to the URL.

We can compress it in viewsreference_views_pre_render using
UrlHelper::compressQueryParameter(json_encode($view->element['#viewsreference']))

And the uncompress it in viewsreference_views_pre_view using
json_decode(UrlHelper::uncompressQueryParameter($view->getRequest()->query->all('viewsreference')), TRUE)

This only adds test coverage which looks pretty good for a first setup. We can always expand/improve later. Merged for now, thanks!

Merged, thanks!

Created a MR. Let's see if this breaks anything.

seanB → created an issue.

I'll check what happens if we remove the patch when we update gin. At a first glance the changes in 🐛 Table header overflow issue for multiple user role on permissions page. Fixed might also fix the issue.

Fixed, thanks!

Fixed, thanks!

Merged! Thanks all.

This seems to do it. Updated the MR.

Would it be possible to move cloneParagraphs, cloneInlineBlocks to a separate service so it can be used in other code/modules? It would also be nice to move the code to clone a specific section to a separate cloneSection method. I'm currently adding a custom controller that duplicates the code to clone a specific section in a layout, and it would be nice to reuse the code we have here.

Tests are all green with the latest changes in the MR. Impact on the amount of queries is the same. From about a 1000 queries to 250 queries for a page with a bunch of layout builder components. 🎉

This caused quite some issues. It probably makes more sense to add the caching to LayoutTempstoreRepository. This solves the issue I experienced with a lot less impact. Updated the MR and IS.

Added a MR. Not sure what the best approach would be to add test coverage. This shouldn't really change anything, except maybe the number of queries to the database. Also would love to get feedback from the framework managers if this is something they would consider adding.

Added a new 3.x release with the changes from the PR https://git.drupalcode.org/project/groupmenu/-/merge_requests/10
Thanks everyone!

We can't simply update the static cache when it is updated, so lets unset it. Sorry about that.

Attached is a patch that adds static caching. This reduced the number of queries in a layout builder edit page from 912 to 227.

Before:

After:

seanB → created an issue.

This seems to make sense! Thanks for the issue. I will check the PR this week and try to get this in.

seanB → created an issue.

Created an MR to fix it. We might want to consider moving the gin_element_info_alter to a different file now it no longer only affects form elements. Did not do that for now though.

seanB → created an issue.

Can we figure out a way to not hardcode the form mode to 'edit'? I'm running into the same issue mentioned in ✨ Use a different form mode when editing block content in an inline block Needs work .

Patch is attached

seanB → created an issue.

seanB → created an issue.

Patch is attached.

seanB → created an issue.

Committed. Thanks all!

This should be fixed by 🐛 Configuration schema for "Enable extra settings" (title,pagination,...) incorrect Fixed

Thanks all! Committed the latest patch.

Thanks for the MR! Also, to weigh in on the discussion about alter hooks vs other ways of changing the parameters: we want the way to alter this to be as lightweight as possible. Cached pages should be served with minimal overhead.

When getting the defined parameters, we can't even rely on everything in the container yet. The module handler works, but I still had to force it to load all .module files. Using the event system could become problematic, same goes for plugins. Dependency injection could easily lead to a circular reference (found that out the hard way while trying to do fancy stuff in my implementation of the hook).

Overall, hooks still have their place in the Drupal universe and I don't see that changing anytime soon.

Patch is attached to implement a hook that allows the parameters to be altered.

Nice work! This was a lot easier to review than the previous PR. So thank you for that.
Added my review to the PR. Hopefully we can find more people to give this a try and thoroughly test it.

After this is fixed I will add this to a new major release. I suggest we create a 3.x branch to follow the versioning of the groups module.

Reroll for latest version.

Attached is an updated patch for Drupal 10.2.

Attached is a patch that seems to solve the issue.

This doesn't seem to fix the following use case:

• Add the Dutch language (default, no prefix) and English language (with a /en prefix).
• Edit a Dutch node, linking to a different Dutch page using the English admin language.
• When a module calls PathValidator::getUrlIfValidWithoutAccessCheck() for the link, you run into the same issue.

Found this while looking at 🐛 processUrl Cannot Find the Correct Entity in Multilingual Site Active .
We could use the language with an empty language prefix as the default langcode. I think that should fix the issue.

This is indeed an issue. A little more context. The following calls lead to the issue:
HtmlLink::parseEntitiesFromText()
EntityUsageBase::findEntityByUrlString()
EntityUsageBase::processUrl()
PathValidator::getUrlIfValidWithoutAccessCheck()
PathValidator::getUrl()
PathValidator::getPathAttributes()
PathProcessorManager::processInbound()
AliasPathProcessor::processInbound()
AliasManager::getPathByAlias()

The AliasPathProcessor::processInbound() doesn't pass a langcode to AliasManager::getPathByAlias(), and for that reason AliasManager::getPathByAlias() uses the current language from the URL. If you link to a page in another language this is an issue, but also if the langcode in the URL is different for other reasons (like working on translations or something).

The current patch has some issues though.

1. +++ b/src/EntityUsageBase.php
   @@ -9,11 +9,15 @@ use Drupal\Core\Entity\EntityRepositoryInterface;
   +use Drupal\path_alias\AliasManagerInterface;
   

   We don't have a hard dependency on the path_alias module, and we should not add it just for this bug.

2. +++ b/src/EntityUsageBase.php
   @@ -363,4 +402,29 @@ abstract class EntityUsageBase extends EntityTrackBase {
   +  protected function processUrlForAllLanguages(string &$url): void {
   +    $languages = $this->languageManager->getLanguages();
   +    $request = Request::create($url);
   +    $resolved_path = $this->pathProcessorManager->processInbound($url, $request);
   +    foreach ($languages as $language) {
   +      $alias = $this->aliasManager->getPathByAlias(
   +        $resolved_path,
   +        $language->getId());
   

   This works around the issue, but duplicates a bunch of code. The real issue is the LanguageNegotiationUrl inbound path processor strips the language prefix from the URL, and the AliasPathProcessor is not able to pass the langcode (when available). It seems there already is a core issue to fix this: #3314941: Language-prefixed alias paths cannot be correctly routed inside a request →

Could you try if #3314941: Language-prefixed alias paths cannot be correctly routed inside a request → fixes the issue?

I think doing something like ✨ Alter hook to adjust image effect Needs review makes a bit more sense. It would reduce the complexity of this module and add more flexibility.
Do you think a hook would also give you what you need?

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

Merged! Thanks.

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

I see no harm in doing this, I'll check it out soon. If anyone else can test and RTBC it before I get to it, this would also help!

Busted 🙈! Thanks for this. Merged.

Could someone create an MR for this? This seems like a reasonable change.

This seems to be a duplicate of 📌 Fix the issues reported by phpcs Needs work , which also addresses some other CS issues.

At a first glance I think the subgroup support being in the same MR as the support for groups v2/v3 makes it a bit harder to review.
Not sure now what is absolutely necessary for the upgrade, and changes made to support the new features.

Most of the times it is probably best to keep MRs as small as possible and create separate issues for new features. As for the next steps, I would recommend we try to get these in first:
📌 Fix the issues reported by phpcs Needs work
📌 Drupal 10 compatibility issues Needs review

After that this PR should already be a bit smaller. If we can then create separate issues for the news features (like subgroup support), and split the MR up, I think we should be able to get everything merged more easily.

Thanks for this. There is 1 thing that I noticed, the entity type manager is now injected when we explicitly didn't do that before to prevent a circular dependency.

kristiaanvandeneynde → credited seanB → .

Has this been fixed by #3032078: Multiple webheads can cause infinite growth of Twig cache → ?

Ah yes, the string|int|null user ID is definitely troublesome. We either cast to int for both values, or change the strict comparison. I don't have a string opinion on it either way. I feel casting to int is slightly nicer, but not sure if that could lead to other unforeseen issues.

So either:
$is_owner = $account->id() && (int) $account->id() === (int) $entity->getOwnerId();

Or simply just do:
$is_owner = $account->id() && $account->id() == $entity->getOwnerId();

The node modules contains this by the way, so maybe to non-strict comparison would be more in line with that:

$uid = $node->getOwnerId();
if (... && $account->isAuthenticated() && $account->id() == $uid)

Tomorrow is my day off, but I'll run the latest version through our tests to see if we can remove the custom cache resets. Thanks for working on this. The content overview was kind of slow taking over 10 sec for some users. With the patch it went down to like 4/5 sec, so pretty significant improvement.

Found a small issue with my latest patch, this should fix it!

Rerolled the patch for 2.x.

Unfortunately, logged-in users were still running into pages where they didn't have the admin toolbar. It would be nice if we could skip Fast404 for authenticated users entirely.

Attached is a patch that would fix this.

seanB → created an issue.

The attached patch seems to solve (at least part) of the issue for us. It's probably not a long-term solution, so just uploading this in case it helps anyone.

Rerolled #21 for the latest dev version.