Netherlands
Account created on 6 July 2009, about 15 years ago
  • Contractor at iOΒ  …
#

Merge Requests

More

Recent comments

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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?

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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?

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands
+++ 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);
πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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?).

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands
πŸ‡³πŸ‡±Netherlands seanB Netherlands

Removed all the Drupal CI scheduling.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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)

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands
πŸ‡³πŸ‡±Netherlands seanB Netherlands

Merged, thanks!

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

This seems to do it. Updated the MR.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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. πŸŽ‰

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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:

πŸ‡³πŸ‡±Netherlands seanB Netherlands

seanB β†’ created an issue.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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 .

πŸ‡³πŸ‡±Netherlands seanB Netherlands

Patch is attached

πŸ‡³πŸ‡±Netherlands seanB Netherlands

Thanks all! Committed the latest patch.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

Reroll for latest version.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

Attached is an updated patch for Drupal 10.2.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

Attached is a patch that seems to solve the issue.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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?

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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?

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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!

πŸ‡³πŸ‡±Netherlands seanB Netherlands

Busted πŸ™ˆ! Thanks for this. Merged.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands
πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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)
πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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

πŸ‡³πŸ‡±Netherlands seanB Netherlands

Rerolled the patch for 2.x.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

Attached is a patch that would fix this.

πŸ‡³πŸ‡±Netherlands seanB Netherlands

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.

Production build 0.69.0 2024