Disclose flagging view mode in flag theming

+1

Thank you for this. It helps to create template suggestions based on the flaggable view mode. This helps with theming base on the flaggable view mode. Very useful! I wonder whether such template suggestions should not be part of the flag module itself but that could go in a followup.

--- a/modules/flag_count/src/Plugin/ActionLink/CountLink.php
+++ b/modules/flag_count/src/Plugin/ActionLink/CountLink.php
@@ -23,9 +23,9 @@ class CountLink extends AJAXactionLink {
   /**
    * {@inheritdoc}
    */
-  public function getAsFlagLink(FlagInterface $flag, EntityInterface $entity) {
+  public function getAsFlagLink(FlagInterface $flag, EntityInterface $entity, $view_mode) {
     // Get the render array.
-    $build = parent::getAsFlagLink($flag, $entity);
+    $build = parent::getAsFlagLink($flag, $entity, $view_mode);
 
     // Normally, you'd just override flag.html.twig in your site's theme. For
     // this example module, we do something more advanced: Provide a new

For each method where we alter the signature by adding the new $view_mode parameter, we should:

• Strict typing it to string
• Make it optional, defaulting to NULL
• If NULL is received, we should
  • Trigger a deprecation error
  • Log a warning message

  Messages should state that the param will be mandatory in the following Flag major version.

• A change notice should be issued.

@claudiu.cristea could you provide an example of template suggestions? thanks

@kimlop

Maybe something like...

function yourmodule_theme_suggestions_flag_count_alter(array &$suggestions, array $variables): void {
  $suggestions[] = 'flag_count__' . $variables['view_mode'];
}

We've just run into the same issue sadly and need the information about the view mode in the flag template. So it's indeed not an edge-case. Going to push this forward now.

@claudiu.cristea Re #20 I implemented:
✅ Strict typing it to string
✅ Make it optional, defaulting to NULL, otherwise is breaking BC for all sites extending these methods
✅ If NULL is received, we should
✅ Trigger a deprecation error
❓ Log a warning message

✅ Messages should state that the param will be mandatory in the following Flag major version.
🔜 A change notice should be issued.

Could you perhaps take a brief look, if that's what you expected? So we can push things forward here.
I was a bit unsure at which places we should implement the deprecation errors, quite a lot of places.

@idebr maybe you'd also like to take a look at the new MR based on your patch. One test is failing, I'm not yet sure why.

Not sure why the $view_mode argument should default to NULL? Could just be string $view_mode = 'default' to match the current behaviour

@idebr I think I've seen NULL as default also in core for that. Core then falls back to default if needed, if I'm correct. So that seems right to me!

[...] I've seen NULL as default also in core for that. Core then falls back to default if needed [...]

I couldn't find any core code doing that. It is instead probably related to @claudiu.cristea in #20:

[...]
Make it optional, defaulting to NULL, otherwise is breaking BC for all sites extending these methods
If NULL is received, we should
Trigger a deprecation error
Log a warning message
Messages should state that the param will be mandatory in the following Flag major version.
[...]

I'd agree with that suggestion and seeing that the deprecation and logging part isn't implemented yet, and one test still fails, I'll set this to "Needs work" again. I'll have a go at it.

Yes, of course it's related to #20 but I thought NULL was also used in core for that in some places. Might have been D7 then perhaps, where I had seen that in the past.

Core uses "default" or "full" in some places, for example https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/block...

But I'd also vote to keep it NULL until the fallback is removed, I think it's most clear, if it has no other disadvantages.
That said I'm also totally fine with changing this if it has any benefits.

I adjusted and moved the deprecation notices here: https://git.drupalcode.org/project/flag/-/merge_requests/67/diffs?commit...

All methods end up either calling getAsLink() or getAsFlagLink() and since these methods are all public, it only makes sense to throw the deprecation errors there.

That is quite weird. I get the exact same phpunit error locally, when running Drupal\Tests\flag_follower\Functional\FlagFollowerUITest::testUi on the 8.x-4.x dev branch, even if the remote on "main" succeeds: https://git.drupalcode.org/project/flag/-/jobs/568309

Change record can be found here: https://www.drupal.org/node/3458551 → .

Anybody → changed the visibility of the branch 3049155-disclose-flagging-view to hidden.

Alright, all done!

The last remaining test failure is unrelated, as the test also fails on current 8.x-4.x-dev, as can be seen here: https://git.drupalcode.org/project/flag/-/merge_requests/68.

Please review!

Anybody → changed the visibility of the branch 8.x-4.x to hidden.

Great work @Grevil!

The only thing I found left is:

Code Quality scans found 173 new findings and 168 fixed findings.

Can you check that? Of course we shouldn't make unrelated changes / fixes!

Regarding the broken test we can't do anything as of #36. So from my side this is ready for RTBC once the code quality changes have been checked.

Some of the warnings are incorrect, e.g. "Call to method setRouteParameter() on an unknown class Drupal\flag\ActionLink\Url." but others make sense, e.g. using a non injected logger instance.

On the other hand, the logger will be removed, once the deprecated code gets removed in 5.x. Meaning we would need to introduce a new logger object which will be introduced as deprecated... Meaning third party modules would need to take care of a second deprecation...

So all perfect, please RTBC!

Thank you @Grevil, I agree. Apart from that it might make sense to give the module a code-style cleanup in a separate issue using the modern tools.

Adding new parameters to an interface, even when optional, is an API change. The project is beta, but it's been beta forever, so conflicted about adding this.

One option is to not add it to the interface and just the implementations, that is allowed.

Other thoughts:
* Also changes routes to require additional parameters.
* Not sure about the warning log message. That could result in *many* warnings on a busy site. Just the trigger error might be sufficient?
* theme suggestions based on the flaggable id seems overkill. I understand it's already there, but I don't think we need to add even more variations.

Also changes routes to require additional parameters.

All routes using the "view_mode" parameter, have a "view_mode" parameter set. I don't think we need to do anything else here?

theme suggestions based on the flaggable id seems overkill. I understand it's already there, but I don't think we need to add even more variations.

I talked to our theme developer, and he said that at least the first two additions add a lot of useful benefit!

Adjusted everything else. Please review, last changes!

Thanks @Grevil! Just had a look and LGTM! Back to RTBC from my side.

I also agree that at least these suggestions totally make sense to add benefit to the functionality and make it easier to use them without additional custom code for typical cases like they know it from other entities.

I'm slightly unsure about the return type addition in src/ActionLink/ActionLinkTypePluginInterface.php is that fine here?

BTW we're now using this in a project and it works great, thanks all!

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