Loading
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 inOrganizationAccess::remember()andOrganizationGovernance::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 mostOrganizationAssigner::chunkSize()IDs long, becauseBoundedQueriesTestbounds the arguments of every query of the module. organizations(),mapillaryOrganizationIds()andgoverningOrganizations()keep their answers for the request and drop them when the checksum of the list cache tags they depend on changes (the same approach asorganizationIdsByViewAccess()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) holdsDrupal.mapillary.prefersReducedMotion().mapillary/viewerdepends on it andmapillary_leaflet/pickernow does too; the picker could not depend onmapillary/viewerwithout 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 (onzoomstartand inrender()), andfocusNearest()focuses the nearest marker or cluster afterwards, or the map itself when there is none. - The
radial-gradient()of.mapillary-split-marker__coneis wrapped the way stylelint wants it, since Drupal CI runs stylelint.
Documentation
CHANGELOG.mdgets the entries of #3633409, which had none, and of this issue.AGENTS.mdno longer says that the organization filter is a numeric group ID text field or that the hooks callmoduleExists('group')(GroupInstalled::check()does that), and describes the preloading and the motion library.README.mdsays 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 beforemapillary_organization_post_update_uninstall_unused(), which must stay last. OrganizationGovernancetakes 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 beforedrush updatedbkeeps working.GovernedContentTest::testGovernanceWithTheOldConstructor()covers it.Drupal.mapillary.prefersReducedMotion()keeps its name and is still available throughmapillary/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 failedBoundedQueriesTest. - 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()andtestClusterSplitKeepsTheFocusOnTheMap(). - Run locally on Drupal 11.4 with PostgreSQL and Group 3.3.5:
GovernedContentTest(11),BoundedQueriesTest, FunctionalJavascriptMapillaryPickerTest(16) andSplitViewerAccessibilityTest(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_organizationKernel directory (140 tests) ran once, on the first version of the change; onlyBoundedQueriesTestfailed, 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.