Problem/Motivation

Beginning in Drupal 8.7, there is no longer a canonical route for media entities. This caused LinkIt to break and was fixed here: #3040749: Drupal 8.7 no longer has a dedicated canonical route for media entities, breaks entity matcher deriver.

In Media settings, if the 'Standalone media URL' setting is not enabled, then when a user inserts a link to a media entity with LinkIt, the path suggestion returned is '/media/{id}/edit' instead of '/media/{id}'. While the input filter still works, it's confusing to end users to see the '/edit' at the end of the path.

Proposed resolution

In Drupal\linkit\Plugin\Linkit\Matcher\EntityMatcher::buildPath(), check the 'Standalone media URL' media setting and remove '/edit' from the end of the path string before returning.

Remaining tasks

Submit patch.
Community review.

User interface changes

When the 'Standalone media URL' setting is not enabled, the path returned will be '/media/{id}'.

API changes

None.

Data model changes

None.

Release notes snippet

TBD

CommentFileSizeAuthor
#15 linkit_3063393_15.patch637 bytesf2boot
#11 interdiff-3063393-2-11.txt1.74 KBChris Burge
#11 linkit-media_entity_update_canonical_path-3063393-11.patch2.16 KBChris Burge
#9 3063393-9.patch2.5 KBShruthi Shetty M
#2 linkit-media_entity_update_canonical_path-3063393-2.patch1 KBChris Burge
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Chris Burge created an issue. See original summary.

Chris Burge’s picture

Chris Burge CreditAttribution: Chris Burge at University of Nebraska commented
Assigned: Chris Burge » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
linkit-media_entity_update_canonical_path-3063393-2.patch1 KB

Patch attached

Chris Burge’s picture

Chris Burge CreditAttribution: Chris Burge commented

There's no way that composer error is related to this patch. Re-testing

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Isn't media/ID just as confusing when users expect to link to the actual file, doesn't feel like this improves much?

There are many issues around showing the title instead of the link, that might be a better long term solution, but it's challenging.

Chris Burge’s picture

Chris Burge CreditAttribution: Chris Burge commented

If it were up to me, we'd show the name with the internal path in parentheses for all referenced entities. E.g. "2019 Annual Report.pdf (media/45)", "Board of Directors (node/34)". Patch #2 simply conforms to the existing convention.

maursilveira’s picture

maursilveira CreditAttribution: maursilveira commented

Thank you @Chris for the patch!

I tested patch on #2 and it works correctly as intended, removing string "/edit" from Media entity internal path when a standalone URL isn't allowed, and keeping the current behaviour of displaying the entity path.

Although I agree with both @Cris and @Berdir that a solution displaying the entity title/name would be a preferable long term solution, this seems to not be in the scope of this issue.

maursilveira’s picture

maursilveira CreditAttribution: maursilveira commented
Status: Needs review » Reviewed & tested by the community
anon’s picture

anon
Primary language Swedish
Location Gothenburg
CreditAttribution: anon commented
Status: Reviewed & tested by the community » Needs work

We need to add a test for this before commiting it.

Shruthi Shetty M’s picture

Shruthi Shetty M CreditAttribution: Shruthi Shetty M at Specbee commented
Status: Needs work » Needs review
FileSize
3063393-9.patch2.5 KB

Added test.

anon’s picture

anon
Primary language Swedish
Location Gothenburg
CreditAttribution: anon commented
Status: Needs review » Needs work

@Shruthi I dont see anything actually testing this.

There are no asserts. Am I missing something?

Chris Burge’s picture

Chris Burge CreditAttribution: Chris Burge at University of Nebraska commented
Status: Needs work » Needs review
FileSize
linkit-media_entity_update_canonical_path-3063393-11.patch2.16 KB
interdiff-3063393-2-11.txt1.74 KB

Patch #2, updated here with tests, is attached.

There was a logic bug in patch #2 that writing tests helped me catch. See the interdiff. We should only be stripping '/edit' if 'standalone_url' is enabled. The initial patch had that logic backwards.

  • anon committed d1aecdb on 8.x-5.x authored by Chris Burge 
    Issue #3063393 by Chris Burge, maursilveira, anon: Remove '/edit' from...
anon’s picture

anon
Primary language Swedish
Location Gothenburg
CreditAttribution: anon commented
Status: Needs review » Fixed

@Chris Burge, thanks for the patch. Commited now.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

f2boot’s picture

f2boot CreditAttribution: f2boot as a volunteer commented
Category: Feature request » Bug report
FileSize
linkit_3063393_15.patch637 bytes

Seems to me there is a bit of over correction here.

Suggestion for media in ckeditor are not working any more because path is truncated of 5 characters.
Quick fix is to double check about existence of "/edit" at the end of the path
But I wonder if there is not also a problem of logic:
[I have "standalone URLs allowed"]
According to comment, shouldn't we negate $standalone_url in condition ?

Thanks

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at iO commented

Yup, -dev HEAD is broken for media because of this. Opened a new issue for this because we can't reopen this one.
#3108030: Media links with standalone_url on are broken