Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
In a view, flag behaves as a generic link though in flag settings "AJAX link" is chosen.
Comment | File | Size | Author |
---|---|---|---|
#23 | flag-broken_ajax_link_in_views-2846111-23.patch | 2.29 KB | Oleksiy |
|
Comments
Comment #2
kpv CreditAttribution: kpv commentedComment #3
joachim CreditAttribution: joachim as a volunteer commentedComment #4
tbrix CreditAttribution: tbrix commentedI 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.
Comment #5
joachim CreditAttribution: joachim as a volunteer commentedThanks for having a go at this.
It doesn't look quite right to me though:
Why do we need to special case a particular link type plugin?
Comment #6
tbrix CreditAttribution: tbrix commentedSorry 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 ?
Comment #7
sagesolutions CreditAttribution: sagesolutions commentedApplied patch #4 and it worked for me.
Thanks!
Comment #8
iMiksuTried the patch and it seems to fix the issue.
Comment #9
socketwench CreditAttribution: socketwench commentedI thought so too, but it seemed to blow up the entire module. Not sure why, though.
Comment #10
joachim CreditAttribution: joachim as a volunteer commentedBlow up in what way....?
Comment #11
socketwench CreditAttribution: socketwench commentedHmmm, 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.
Comment #13
joachim CreditAttribution: joachim as a volunteer commentedThanks!
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.
Comment #14
martin107 CreditAttribution: martin107 as a volunteer commentedI 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.
Comment #15
joachim CreditAttribution: joachim as a volunteer commentedhttps://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.
Comment #16
socketwench CreditAttribution: socketwench commentedRetesting now that the branch failures are resolved.
Comment #17
socketwench CreditAttribution: socketwench commentedHuh. So that works according to the tests. Does this solve the original issue? is there any way we can simplify this code further?
Comment #18
socketwench CreditAttribution: socketwench commentedThe #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.
Comment #19
joachim CreditAttribution: joachim as a volunteer commentedMy comment #5 still stands.
Comment #20
rasikap CreditAttribution: rasikap commentedThe patch in #11 works for me.
Comment #21
madhukunj CreditAttribution: madhukunj as a volunteer commentedI 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
Comment #22
joachim CreditAttribution: joachim as a volunteer commentedThere is no point in people commenting to say the patch works -- further work is needed on it as stated in #5.
Comment #23
OleksiyUpdated 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.
Comment #24
socketwench CreditAttribution: socketwench commentedThis seems like much more the correct approach, it parallels the approach in `flag_entity_view()`, although the render array looks different:
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.
Comment #25
andypost+1 to use lazy builder
Comment #26
Manoj Raj.R CreditAttribution: Manoj Raj.R as a volunteer commented#4 Worked for me
Comment #27
kerasai CreditAttribution: kerasai at net2Community, Inc. commentedConfirming that the patch in #23 works as expected--the link in the view renders as AJAX-enabled and successfully performs the AJAX operation.
Comment #28
OleksiyI 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.
Comment #29
andypost@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
Comment #30
socketwench CreditAttribution: socketwench commentedThe 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.
Comment #32
socketwench CreditAttribution: socketwench commentedThanks everyone!