Media entity parent link does not support Commerce Product entity

Created on 25 February 2024, 9 months ago
Updated 9 September 2024, 2 months ago

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

✨ Feature request
Status

RTBC

Version

1.0

Component

Code

Created by

🇨🇦Canada harismalik

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @harismalik
  • 🇩🇪Germany stefan.korn Jossgrund

    @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.

  • 🇩🇪Germany stefan.korn Jossgrund
  • 🇩🇪Germany stefan.korn Jossgrund
  • Assigned to stefan.korn
  • 🇩🇪Germany stefan.korn Jossgrund

    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.

  • Issue was unassigned.
  • Status changed to Needs review 7 months ago
  • 🇩🇪Germany stefan.korn Jossgrund

    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

  • 🇩🇪Germany stefan.korn Jossgrund
  • Status changed to RTBC 3 months ago
  • 🇩🇪Germany stefan.korn Jossgrund
Production build 0.71.5 2024