Problem/Motivation

Currently, any Drupal Commerce sites that contain media fields for product images cannot use this module to link images to their product variations using this module. This module currently only checks for 'canonical' link templates. The ProductVariation entity does not contain a value for canonical under the array of link templates in the annotation for the entity definition.

Steps to reproduce

  1. Create a commerce product variation with a media image.
  2. Enable Media Entity Parent Link module
  3. Enable the "Link to parent entity (if applicable)" setting inside the image format settings under "Manage Display".
  4. Save the display settings.
  5. Click on a product variation image.
  6. Nothing happens

Proposed resolution

Simply check for link templates 'collection' and 'canonical'

Remaining tasks

Improve or merge patch/PR.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

harismalik created an issue. See original summary.

harismalik’s picture

Title: Media entity parent link cannot work with Commerce Product entity. » Media entity parent link does not support the Commerce Product entity
harismalik’s picture

Title: Media entity parent link does not support the Commerce Product entity » Media entity parent link does not support Commerce Product entity
harismalik’s picture

Patch file for immediate implementation.

stefan.korn’s picture

@harismalik. Thanks for pointing this out. Interesting problem. I tried it out too.

First of all, I suppose the steps to reproduce would be

1. Create a commerce product variation with a media image.
5. Click on a product variation image.

Because just with a product this seem to work with the canonical link.

Now looking at the product variation entity, I suppose it doesn't especially matter if the collection link template does exist. It just matters that the product variation entity does not provide a canonical link template and it would be sufficient here to just check if it is product variation entity.
So something like
if (!$parententity->isNew() && ($parententity->hasLinkTemplate('canonical') || $parententity->getEntityTypeId() === 'commerce_product_variation'))
would probably do it as well.

But it seems a bit odd to just check for the entity type, a more generic solution would be better imho.

The product variation entity provides a method toUrl which handles the creation of a "canonical" link. So basically the product variation has a canonical link, but not provided as link template.

So for a more generic solution I am thinking if
if (!$parententity->isNew() && method_exists($parententity, 'toUrl'))
could do it. It works for the case given with the product variation. But at this moment I am not exactly sure if this has some undesired implications.

On the other side, I am not sure why product variation does not provide a canonical link template (probably because it just uses query params to alter from product url) and whether maybe commerce could provide a canonical link template for product variation entity as well.

stefan.korn’s picture

stefan.korn’s picture

stefan.korn’s picture

Assigned: Unassigned » stefan.korn

There seem to be related issues. The check for "canonical" template was introduced to mitigate a problem with non linking entities (paragraphs). So there is probably an issue if just checking for method toUrl, probably any entity has this method.

The paragraph entity is throwing an exception for the canonical link template. Probably a solution would be to get away from checking the "canonical" template and just getting toUrl on the parent entity but put this in try/catch to avoid exception and just do not put out a link in case the exception is caught. That way behavior would be like before, non error on non linkable parent entities, but the toUrl would work independently whether the template is canonical or something else. That will be more flexible and would solve this issue in a generic way probably.

Thinking about this and testing, but seems like a good approach.

  • stefan.korn committed f717b193 on 8.x-1.x
    Issue #3423688: Media entity parent link does not support Commerce...
stefan.korn’s picture

Assigned: stefan.korn » Unassigned
Status: Active » Needs review

So I have committed a more generic approach to the dev branch, that should also handle commerce product variant correctly.

On my end it is working.

Would be great if you can test it too. @harismalik

stefan.korn’s picture

stefan.korn’s picture

Status: Needs review » Reviewed & tested by the community
harismalik’s picture

Issue summary: View changes
stefan.korn’s picture

Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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