FlagFollowerUITest failing since new access policy was committed to core

Created on 13 July 2024, about 2 months ago
Updated 25 August 2024, 14 days ago

Problem/Motivation

Tests are failing since πŸ“Œ Implement the new access policy API Needs work .

\Drupal\Tests\flag_follower\Functional\FlagFollowerUITest::doContentView() specifically, we're missing some kind of cache context apparently.

It's a user placeholder, specifically on a relationship of a view: \Drupal\flag\Plugin\views\relationship\FlagViewsRelationship::query().

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

πŸ“Œ Task
Status

Fixed

Version

4.0

Component

Flag core

Created by

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Berdir
  • Assigned to huzooka
  • πŸ‡­πŸ‡ΊHungary huzooka Hungary πŸ‡­πŸ‡ΊπŸ‡ͺπŸ‡Ί

    Started working on this. So far, the view in flag_following seems to be quite dated, based on git blame.

  • Pipeline finished with Success
    about 1 month ago
    Total: 147s
    #241982
  • πŸ‡­πŸ‡ΊHungary huzooka Hungary πŸ‡­πŸ‡ΊπŸ‡ͺπŸ‡Ί

    What I did so far:

    1. I think that @Berdir's hint about the cacheability is right. I changed FlagViewsRelationship to implement CacheableDependencyInterface, and implemented the necessary methods:
      1. If we're asked for a per-user relationship and the configured flag is not global, then the cache context will be ['user]
      2. Left the cache max age -1 (permanent)
      3. ...but in the cache tags, we return with the flag's cache ID (so if the flag will be changed somehow to be non-global or vice versa, then the view be recalculated).
    2. Installed flag_following on a vanilla Drupal (10.3.x) and copied the re-exported YAML back to the module (without the config hash parts)
    3. Then I cleaned up the changes in the file to help reviewing the changes... But maybe we don't want this cleanup?..
    4. Fixed the two PHP CS nits in the plugin class.

    I don't know whether it is needed to add some test coverage for the relationship plugin, let's see a green result for first (and ideally, some community review or hits from maintainers).

  • Issue was unassigned.
  • Status changed to Needs review about 1 month ago
  • πŸ‡­πŸ‡ΊHungary huzooka Hungary πŸ‡­πŸ‡ΊπŸ‡ͺπŸ‡Ί

    It is green, so try to get a review for now.

  • Pipeline finished with Skipped
    14 days ago
    #263970
  • Status changed to Fixed 14 days ago
  • πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

    Thanks!

Production build 0.71.5 2024