Disclose flagging view mode in flag theming

I can confirm I've also experienced this issue, I've resolved the situation by adding the following process using hook_theme_suggestions_alter()

/**
 * Implements hook_theme_suggestions_alter().
 */
function my_module_theme_suggestions_alter(array &$suggestions, array $variables, $hook) {
  if ($hook === 'flag') {
    $route_name = Drupal::routeMatch()->getRouteName();
    // Get visited node object if exist.
    $node = Drupal::routeMatch()->getParameter('node');
    // Get flagged node from flag field hook variables.
    $flagged_node = $variables['flaggable'];
    if (!isset($node)) {
      // In case of using flag AJAX action link.
      // Within ajax Drupal::routeMatch()->getParameter('node') is null because
      // The current route is flag route and not node full page.
      // The solution here is to get the node from the HTTP referer path.
      $http_referer = Drupal::request()->server->get('HTTP_REFERER');
      $path = parse_url($http_referer)['path'];
      $langcode = Drupal::languageManager()->getCurrentLanguage()->getId();
      $path = str_replace('/' . $langcode . '/', '/', $path);
      $path_alias = \Drupal::service('path.alias_manager')->getPathByAlias($path);
      if(preg_match('/node\/(\d+)/', $path_alias, $matches)) {
        // Load the visited node (if exist) from node path.
        $node = Drupal::entityTypeManager()->getStorage('node')
          ->load($matches[1]);
      }
    }
    if ($node instanceof NodeInterface && $flagged_node instanceof NodeInterface) {
      if ($flagged_node->id() === $node->id()) {
        $suggestions[] = 'flag__' . $variables['flag']->id() . '__full';
      }
    }
  }
}

Attached patch adds an additional argument 'view mode' to the flag link builder to enable conditional logic in the theming layer.

Attached patch fixes the flag response when using ajax links.

Patch works as designed, I now have a view mode.

Attached patch fixes the flag_link Views field.

I've tried patch #8, and it works on the initial load, but the view mode information is lost after triggering an Ajax reload by (un-)flagging. In my case I first had a flag--full.html.twig suggestion on the intial load, but after clicking the link, I got a flag--default.html.twig suggestion instead.

Well I got now the possibility to create a new view mode for the Flags, but how do I apply this view mode in my Node entity ? There is no option in the Format.

And if I crate a suggestion in a preprocess, it's not working after Ajax submit as Hudri said.

#13 The view mode is added implicitly when the Flag link is rendered, similar to fields. There is no configuration required.

Attached patch adds the following change compared to #8:

1. The view mode now persists when using AJAX flag links

Not sure what is going on with the merge request linked to this issue. I suggest using this patch instead.

Attached patch adds the following change compared to #14:

1. The view mode is now also included in Contextual links, also fixing the test failure

I tested the above patch, the view mode now persists when using AJAX flag links.

Tested patch #16, works great!

One issue that I encountered was when using Conditional Confirm Form as Linktype (using the flag_conditional_confirm module), that combination throws a fatal error related to the code in flag_conditional_confirm.

+1

Thank you for this. It helps to create template suggestions based on the flaggable view mode, allowing to have different theming on different view displays. 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, 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.

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

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!

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!