Regression: API Hook No Longer Works

Pavlos.Dontas → made their first commit to this issue’s fork.

This is going to be tricky...

@lostcarpark and I discussed this and believe we have a solution...

Here's the code in question:

          $link = $entity->toLink($more);
          $project_link = $link->toRenderable();
          $project_link['#attributes']['class'] = [$class];

          // Allow other modules to modify the read more link before it's
          // created.
          $this->moduleHandler->invokeAll('smart_trim_link_modify', [
            $entity,
            &$more,
            &$link,
          ]);

The issue raised by @PapaGrande is valid. $more has already been used by the time the hook is invoked, so any changes that a hook implementation makes to $more are not used.

But, if we move the first three lines of code in the snippet above to below the hook invocation, then $link is being passed to the hook before it is even defined!

I think we are going to modify the code above with the following:

// Allow other modules to modify the "More" text.
          $this->moduleHandler->invokeAll('smart_trim_more_modify', [
            $entity,
            &$more,
          ]);

          $link = $entity->toLink($more);

          // Allow other modules to modify the read more link before it's
          // created.
          $this->moduleHandler->invokeAll('smart_trim_link_modify', [
            $entity,
            $more,
            &$link,
          ]);

Summarizing the code above:

1. Add a new smart_trim_more_modify() hook that is for modifying the "More" text.
2. Modify the existing smart_trim_link_modify() to pass $more by value so it cannot be changed. The alternative is removing the $more argument which would likely break existing hook implementations.

Thoughts?

-mike

I think that works, but I'm wondering if that will be fully backwards compatible. (In my case, I don't actually have any hooks implemented.) Are there scenarios where passing $more by value will be unexpected in an existing hook?

And if this is a breaking change for existing hook implementations, does that mean the change should be reverted and a new major version released?

@PapaGrande - well, the way it is currently written in 2.1.0 is broken - changing $more in a hook implementation does nothing.

In 2.0.0, it was also broken, as changing $uri in a hook implementation didn't do anything (see here). Granted, $more was working at this point.

The change I've proposed will fix both of these, albeit with the potential breaking change that you mentioned. The proposed fix tries to mitigate this by keeping the hook signature the same, but making it clear (by not passing $more by ref) that changing $more in smart_trim_link_modify() will not do anything.

IMHO, this will limit breaking changing, while solving both issues in a meaningful way.

-mike

Dual-hook solution in MR. D10 tests are passing, D9 tests will pass once 🐛 Read more link always displayed in some settings Fixed is merged. Needs review and feedback.

-mike

I just realized there is a better solution without changing the hook signature and only adding one line of code.

          // Allow other modules to modify the read more link before it's
          // created.
          $this->moduleHandler->invokeAll('smart_trim_link_modify', [
            $entity,
            &$more,
            &$link,
          ]);

          // Add this to update the $link if the $more was changed in a hook implementation.
          $link->setText($more);

I'll update the MR (and associated test) in the next day or two.

-mike

MR updated with improved logic - all tests passing except D9 which should pass once 🐛 Read more link always displayed in some settings Fixed is merged.

-mike

Said merging of https://www.drupal.org/project/smart_trim/issues/3365797 🐛 Read more link always displayed in some settings Fixed caused the need for a rebase..

Turns out I am a big boy who can run his own rebases... waiting for test results..

Tests failed after rebase.

Interesting that it was the D9 test that failed. I'll try to take a look at things in the next few days.

-mike

D9 test fixed. Ready for review.

-mike

Looks good. Merging and adding to the release.

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