Munich
Account created on 11 January 2018, over 7 years ago
#

Merge Requests

More

Recent comments

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Link items should have those options, provided that Linkit's widget has been used in the node entity form.
Maybe this was not the case in your scenario?

Your suggested helper method is pretty similar to the existing helper method LinkitHelper::getEntityFromUri($uri), but I have to say that the whole concept of this feature is about the existence of the UUID data attribute, which provides proof that a node had been found by the LinkitWidget at the time the node had been edited.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

@schillerm, I can't reproduce your issue. It is working for link fields as well as links in text fields in my setup.
The method isEntityLoadable() simply checks whether the appropriate data attributes (type and UUID) are present, so the call in line 124 should not be negated.
I renamed it to hasEntityAttributes() to better reflect this behavior and avoid misunderstandings.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Realized that doing an access check will lock out users that might be able to view an entity after login.
Now checking the publish status instead.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

This sounds exactly like the same issue I described in #3534699-14: Refactor plugin for CKEditor5 v45+ β†’ .

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

@mark_fullmer, see my comment in #3534699-14: Refactor plugin for CKEditor5 v45+ β†’ for another bug that is related to CKEditor 45. Not sure if this is the right place to report, but as @jannakha points out in their reply, it likely has to be fixed in Linkit.

Of course, I can also open a separate issue if that is preferred.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Using the patch from MR37 together with Linkit 7.0.8, which added a similar change, I am experiencing some strange behavior.

I have a link like this:

<a href="/node/30" data-entity-type="node" data-entity-uuid="85f7822b-f889-4515-9e20-1addb7ef8e57" data-entity-substitution="canonical">FOOBAR</a>

When I click on it and edit an attribute (e.g. "Open in new window"), the link is temporarily split up into 3 links: one for the part to the left of the cursor, one invisible filler link (containing only 7 &NoBreak; characters), and one for the part to the right of the cursor.

<a class="ck-link_selected" href="/node/30" data-entity-type="node" data-entity-uuid="85f7822b-f889-4515-9e20-1addb7ef8e57" data-entity-substitution="canonical" target="_blank">FOO</a>
<a class="ck-link_selected" href="/node/30" target="_blank">⁠⁠⁠⁠⁠⁠⁠</a>
<a class="ck-link_selected" href="/node/30" data-entity-type="node" data-entity-uuid="85f7822b-f889-4515-9e20-1addb7ef8e57" data-entity-substitution="canonical" target="_blank">BAR</a>

When the link dialog is closed, the 3 links are replaced with one link again.

<a href="/node/30" data-entity-type="node" data-entity-uuid="85f7822b-f889-4515-9e20-1addb7ef8e57" data-entity-substitution="canonical" target="_blank">FOOBAR</a>

@jannakha, do you have any idea why this happens?

While it normally shouldn't be a problem for most users, it messes up the display for us, because we add an icon to the text through CSS to links that open a new window.
Thus, the text temporarily looks like this:

FOO β†— β†—BAR β†—
(should be FOOBAR β†—, of course)

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

This introduced a backwards incompatible change in the MigrateExecutable class constructor.

Now the following

$executable = new MigrateExecutable($migration, new MigrateMessage(), ['limit' => 10]);

fails with this error:

TypeError: Drupal\migrate_tools\MigrateExecutable::__construct(): Argument #3 ($keyValue) must be of type Drupal\Core\KeyValueStore\KeyValueFactoryInterface, array given
πŸ‡©πŸ‡ͺGermany mrshowerman Munich

I like the idea with the filter setting.
For getting this to work with custom substitution plugins, we shouldn't hard-code the list of available options to canonical and media, and instead fetch all available media substitution plugins.
I added a commit to the MR that does this.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Created an MR which adds a permission to bypass the access check.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

There was a similar issue in Gin when editing paragraphs: πŸ› Paragraphs stick out from the edge Active
It is caused by CKEditor items in table cells that don't have an explicit width.

I created an MR that applies flexbox to the table rows and calculates the optimal width for the cells possibly containing the CKEditors.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

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

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

This has been used for a week in production now, and we didn't notice any issues.
So RTBC+1 from my side.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Earlier today, I updated the patch we use in one of our projects to the latest version of the MR.
Looks good at first sight, but I'd like to watch it a bit longer until giving feedback here.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

I simply removed the maxlength attribute from both form fields.
I don't think it makes sense to increase the value to something random that might still be to small in some cases. There is no technical reason to limit the length.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

The organization part is now reset before we pass the address to the formatter.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

mrshowerman β†’ changed the visibility of the branch 3521673-address-links-remove to hidden.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Nonsense. Issues shold always be targeted against the most current branch and cherry-picked to previous ones if necessary.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

I'll take a look at this.

Not sure if this is a bug or a feature request, thus assigning to the 2.4.x-dev branch.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

I have the same issue. Patch #2 fixes it. RTBC+1.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Tests are green again.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

mrshowerman β†’ changed the visibility of the branch 3048458-preload-attribute to active.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

You have to tick the checkboxes below "Credit & committing" if you want to give credit πŸ˜‰

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Not sure which changes you're referring to, but I rebased anyway.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

I've decided to create an MR in this case so we can better track updates to the branch.

There is no update script that enables the new module so far; I leave the decision of whether to add one to the maintainers.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

mrshowerman β†’ created an issue.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Attached patch fixes both issues.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Rebased an addressed the @larowlan's comment.
Test fail seems unrelated.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Fixed phpcs, back to previous state.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

I noticed that in the HTML method, there were a few attributes added to the link that aren't valid HTML, like type="mailto". On the other hand, other valid attributes got lost in the pre and post keys that were not processed properly.

Created an MR that fixes both on top of the original patch.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

+1 for this change.

There is a typo in the settings form description (missing comma), and the text could be optimised a little:

If checked, a button to rotate the image will appear next to each image.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Re #66: I agree that the 2 checkboxes should be moved, but since it concerns not only the new "Plays inline" box which this issue is about, but also the already existing "Muted", I'd vote for creating a separate issue for this to avoid scope creep.

We've been using this patch for a long time in various projects, and from my point of view, it is good to go.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Did a rebase. Now there's a few phpstan errors again, maybe @mohit_aghera can look into them.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Thanks a lot for the quick fix πŸ™πŸ»

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Thanks for adding the target branch!
Rebased.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

I rebased the MR.
@swentel, I added an example to the IS.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

This should be targeted against 2.4.x-dev, but that's not yet available in the dropdown.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Rebased the MR and created a CR draft.
Also updated the deprecation target to 11.2.0 and the removal target to 12.0.0.
Please review.

Not sure if we need a followup for the removal of the deprecated interface.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Thanks for merging this, @finex!
But shouldn't this issue be fixed then?

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Thanks for the feedback, @anybody. I applied your suggested changes.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Nice! Thanks for all the work, @mdranove. The new MR fixes my issue.
Having a closer look at the changes, I found a few things that could be improved, and took the liberty of changing them myself.

I'm also attaching a static patch that can be applied against 6.0.6 in case anyone needs it.

This looks good to go for me, thus setting in RTBC.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

MR no longer applies, but it looks like the change from this issue has been included in πŸ› Create revision before changing values, set revision author and time Active , so this might be closed as a duplicate.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

I was able to further reduce duplicate code by moving some parts of the viewElements() method into new trait methods. The slight differences between SvgImageFormatter and SvgResponsiveImageFormatter we noticed in πŸ“Œ Formatters shouldn't repeat core code Needs review today were respected, especially the cache tags handling πŸ˜‰

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

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

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Created a new MR that addresses all open issues.
Not sure what the right status should be now πŸ™ˆ

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Closing this as it was decided to work on this in the original issue.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

@mrshowerman not sure we need to create a new issue for that, just open a new MR here with the fix.

Oops, missed that. I created πŸ› Fatal error when linking SVGs to file Active in the meantime, but I'll be happy to add the fix to this issue if that is preferred.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

@mably, I will also have a look into ✨ Use a trait to reduce code duplication in formatters Active .

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Is there an open issue for this already?

Not yet. I had a quick look at this yesterday and was able to reproduce. I also have a fix ready.
Will create a new issue with that fix soon.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Addressed feedback by @grevil.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

I made a few tests on several of our sites using different versions of Drupal (9.5.11, 10.2.0, 10.2.5, 10.2.7, and 10.3.6). All of them show the same error message when saving an unpublished node that has a URL alias. So I can confirm that πŸ› hook_node_grants implementations lead to a 'URL Alias' validation error when saving translated nodes. Fixed did not fix the issue.

I noticed that there is a second workaround: granting the "view any unpublished content" (provided by the module) also fixes the issue.

Updated the IS accordingly.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

All feedback has been addressed, except for one thing where @mably requests input from @Grevil. Setting back to NR.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Rebased.
@sagesolutions, you should be able to test this now against 3.x-dev.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Fixed a bug in the "old" map, showing the wrong number of results and wrong pagination when using the new filter: the attachment view display needs to have the same exposed filters as the parent display.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

@saschaeggi, see my comment on the MR. Leaving status change up to you.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Applied code suggestions and removed a line of code that I think is unrelated.
Back to review for @saschaeggi.

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

The current MR produced a fatal error when using the LinkitFormatter:

ValueError: func_get_arg(): Argument #1 ($position) must be less than the number of the arguments passed to the currently executed function in func_get_arg() (line 24 of modules/contrib/linkit/src/Plugin/Linkit/Substitution/Canonical.php).

That's because the formatter does not pass $options, so I quickly added sanity checks to all occurrences of func_get_arg(1).

Apart from that, I agree with @berdir. I don't understand why we are passing the options to the substitution plugin that creates the URL, and still process the fragment and query parameters afterwards. I also think this processing code has a bug (left a comment on the MR).

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

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

πŸ‡©πŸ‡ͺGermany mrshowerman Munich

Rebased the MR onto the latest 3.x.
@mirakolous, you should now be able to apply the MR's plain diff to 3.x-dev again.

Production build 0.71.5 2024