Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kpv created an issue. See original summary.

kpv’s picture

Issue summary: View changes
joachim’s picture

Priority: Normal » Major
Issue tags: +Release blocker
tbrix’s picture

I am not sure if this is right, but returning $link_type_plugin->getAsFlagLink($flag, $entity) if the plugintype is ajax_link as in this patch works.

joachim’s picture

Status: Active » Needs work

Thanks for having a go at this.

It doesn't look quite right to me though:

+++ b/src/Plugin/views/field/FlagViewsLinkField.php
@@ -122,6 +122,10 @@ class FlagViewsLinkField extends FieldPluginBase {
+      return $link_type_plugin->getAsFlagLink($flag, $entity);

Why do we need to special case a particular link type plugin?

tbrix’s picture

Sorry you are right. It does not look right. I am not clear on why getAsLink is used here and not getAsFlagLink. Rendering via getAsFlagLink also seems to work for the special case added in dev for the FormEntryInterface plugin. Would it be correct to drop the special cases and use getAsFlagLink for all cases here ?

sagesolutions’s picture

Applied patch #4 and it worked for me.

Thanks!

iMiksu’s picture

Tried the patch and it seems to fix the issue.

socketwench’s picture

Would it be correct to drop the special cases and use getAsFlagLink for all cases here ?

I thought so too, but it seemed to blow up the entire module. Not sure why, though.

joachim’s picture

Blow up in what way....?

socketwench’s picture

Status: Needs work » Needs review
FileSize
688 bytes

Hmmm, I can't really remember! I was having one of those tired, frustrated with my utter lack of time for everything kind of days.

So, let's try this patch and see what happens.

Status: Needs review » Needs work

The last submitted patch, 11: viewsAjaxLink_2846111_11.patch, failed testing.

joachim’s picture

Thanks!

A lot of those failures look like they're from the 'extra_permissions' system that was committed recently. So it looks like this patch needs a reroll to account for that.

martin107’s picture

I am sorry, I dont have time at the moment to investigate further .... this comment may or may not be helpful!

are test broken for 8.x-4.x-dev at the moment?

these 61 failing tests are suspiciously similar to failures in separate issue.
#2764709: [Regression/Refactor] Restore Ajax link type "flag message" functionality

as it went from 37 to 36 I only added an extra test and yet similar 'extra_permissions' failure appeared.

Sorry if this is me crying wolf.

joachim’s picture

https://www.drupal.org/node/268362/qa says they pass, but the results are a couple of weeks old. Should be from our most recent commit, but I've queued a test to be sure.

socketwench’s picture

Status: Needs work » Needs review

Retesting now that the branch failures are resolved.

socketwench’s picture

Huh. So that works according to the tests. Does this solve the original issue? is there any way we can simplify this code further?

socketwench’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

The #11 patch does fix the view issue, but we really need test coverage for it. We could leverage the Flag Bookmark test if we were to make the flag an AJAX link instead. Not sure what else that would break, yet, though.

joachim’s picture

My comment #5 still stands.

rasikap’s picture

The patch in #11 works for me.

madhukunj’s picture

I have tested on my Views before Using #11 patch on click on flag link ajax is not working as expected but after applying this patch AJAX link is working fine for me

joachim’s picture

There is no point in people commenting to say the patch works -- further work is needed on it as stated in #5.

Oleksiy’s picture

Updated patch where the flag.link_builder service uses for link generation. Not sure that it's the correct way to solve the issue. Please review.

socketwench’s picture

Status: Needs review » Needs work

This seems like much more the correct approach, it parallels the approach in `flag_entity_view()`, although the render array looks different:

$build['flag_' . $flag->id()] = [
      '#lazy_builder' => ['flag.link_builder:build', [
        $entity->getEntityTypeId(),
        $entity->id(),
        $flag->id(),
      ]],
      '#create_placeholder' => TRUE,
    ];

We're calling the same method though, so it might be right. We still need tests for this, but I think we're on the right track.

For the tests, I'm wondering if we should just modify one of the views in either flag_follower or flag_bookmark and make that an AJAX link.

andypost’s picture

Manoj Raj.R’s picture

#4 Worked for me

kerasai’s picture

Confirming that the patch in #23 works as expected--the link in the view renders as AJAX-enabled and successfully performs the AJAX operation.

Oleksiy’s picture

I can't reproduce the issue on clear Drupal 8 (8.6.x) + Flag module (8.x-4.x) installation. It's also OK with AJAX for a view option enabled.
Maybe someone can share a view config export where this issue can be reproduced.

andypost’s picture

@Oleksiy If there is no bug then this issue is a Task to use lazy builder for views

@socketwench It makes sense to "normalize" how flag links are build, and common service is good way ahead

socketwench’s picture

Status: Needs work » Reviewed & tested by the community

The lazy builder still works, and the work in #2764709: [Regression/Refactor] Restore Ajax link type "flag message" functionality actually covers the need for tests, so let's get this in.

  • Oleksiy authored abf2126 on 8.x-4.x
    Issue #2846111 by Oleksiy, socketwench, tbrix, joachim, kpv, andypost,...
socketwench’s picture

Status: Reviewed & tested by the community » Fixed

Thanks everyone!

Status: Fixed » Closed (fixed)

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