task: #3634264: Reduce the per-row cost of organization access checks and finish the follow-ups of recent issues

Closes #3634264

Checking access to Mapillary content in organizations no longer costs a handful of queries for every row of a page: a page of images, sequences, map features or detections reads the organizations of all its rows at once and remembers the answers for the request. The same issue closes the small follow-ups left by [#3633409], [#3633965] and [#3634056].

Changes

Access checks (mapillary_organization)

  • AccessHooks::entityLoad() (hook_entity_load) only notes the IDs of loaded Mapillary content in OrganizationAccess::remember() and OrganizationGovernance::remember(); nothing is queried there, so importers and other code that never check access pay nothing.
  • The first access check of an entity reads the organizations of that entity and of the others noted with it with one query (OrganizationAccess::organizations()), and the Mapillary organization IDs behind sequences, map features and detections with one query for each path (OrganizationGovernance::mapillaryOrganizationIds()). The lists are at most OrganizationAssigner::chunkSize() IDs long, because BoundedQueriesTest bounds the arguments of every query of the module.
  • organizations(), mapillaryOrganizationIds() and governingOrganizations() keep their answers for the request and drop them when the checksum of the list cache tags they depend on changes (the same approach as organizationIdsByViewAccess() from #3633409). The update and delete checks, which start with the view check, no longer repeat the lookups.
  • Who may view, edit and delete what does not change; the existing access tests pass as they were.

Map picker and split viewer

  • New library mapillary/motion (js/mapillary-motion.js) holds Drupal.mapillary.prefersReducedMotion(). mapillary/viewer depends on it and mapillary_leaflet/picker now does too; the picker could not depend on mapillary/viewer without loading MapillaryJS.
  • The picker's cluster groups are created with animate: !Drupal.mapillary.prefersReducedMotion().
  • A cluster that has the focus is replaced when the map zooms or the markers are drawn again. rememberClusterFocus() keeps its place on the map (on zoomstart and in render()), and focusNearest() focuses the nearest marker or cluster afterwards, or the map itself when there is none.
  • The radial-gradient() of .mapillary-split-marker__cone is wrapped the way stylelint wants it, since Drupal CI runs stylelint.

Documentation

  • CHANGELOG.md gets the entries of #3633409, which had none, and of this issue. AGENTS.md no longer says that the organization filter is a numeric group ID text field or that the hooks call moduleExists('group') (GroupInstalled::check() does that), and describes the preloading and the motion library. README.md says that the page of a public organization is open to non-members and describes the cluster focus and reduced motion.

Upgrade

  • No schema or configuration changes. mapillary_organization_post_update_access_preload() is empty: it makes the update runner rebuild the container for the new hook implementation and service argument. It sorts before mapillary_organization_post_update_uninstall_unused(), which must stay last.
  • OrganizationGovernance takes the cache tags checksum as an optional fourth constructor argument. A container compiled before this change builds the service with three arguments; it then remembers nothing and reads one entity at a time, so a site that deploys the code before drush updatedb keeps working. GovernedContentTest::testGovernanceWithTheOldConstructor() covers it.
  • Drupal.mapillary.prefersReducedMotion() keeps its name and is still available through mapillary/viewer.

Differences from the issue summary

  • The summary said a hook preloads the organizations of the entities being loaded. It would run queries for every load, also where nobody checks access, so the hook notes IDs and the first access check reads them.
  • The lists of IDs follow OrganizationAssigner::chunkSize(), which the summary did not mention. An earlier version of this branch read up to 200 IDs at a time and failed BoundedQueriesTest.
  • The old-constructor fallback and its test are not in the summary.
  • The test for the cluster focus zooms with setZoom(): click() on a cluster did not zoom in the test browser, probably because it spiderfies, so it did not replace the focused element.

Testing

  • New: GovernedContentTest::testQueriesDoNotGrowWithTheEntities() loads 2 and then 12 entities of each of the four types and checks view, update and delete for each; the queries for 12 must not outnumber those for 2 (before the change, for images: 19 and 74). GovernedContentTest::testGovernanceWithTheOldConstructor(). MapillaryPickerTest::testClustersDoNotAnimateWithReducedMotion() and testClusterSplitKeepsTheFocusOnTheMap().
  • Run locally on Drupal 11.4 with PostgreSQL and Group 3.3.5: GovernedContentTest (11), BoundedQueriesTest, FunctionalJavascript MapillaryPickerTest (16) and SplitViewerAccessibilityTest (7). phpcs (Drupal, DrupalPractice) and PHPStan level 6 report nothing on the changed PHP files; stylelint with core's configuration reports nothing on the changed CSS, and the Prettier CLI reports nothing on the changed scripts. The Prettier CLI still wants the cone gradient on one line, as before; the stylelint rule wins because Drupal CI runs stylelint.
  • The whole mapillary_organization Kernel directory (140 tests) ran once, on the first version of the change; only BoundedQueriesTest failed, and that is fixed above. It was not run again on the final commit.
  • Not run locally: the whole suite, the functional tests of the organization module, Drupal 12 and MySQL; the pipeline covers them.

Merge request reports

Loading
Loading