Problem/Motivation

NodeGrantDatabaseStorage::buildGrantsQueryCondition doesn't have a scope.

Proposed resolution

Since

  1. The function is not called from anywhere but the class
  2. is not part of NodeGrantDatabaseStorageInterface
  3. is very much database specific

I suggest a protected scope.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because every method should have a scope.
Prioritized changes This is a bugfix that makes this class a tiny bit less brittle.
Disruption Disruptive for contributed and custom modules if they called this method but the interface is not changed so that was bogus code already.
CommentFileSizeAuthor
#7 2401959-7.patch760 byteswim leers
#3 2401959_3.patch1.89 KBchx
prot.patch653 byteschx

Comments

chx’s picture

chx commented

I assigned to xjm as a node access maintainer.

chx’s picture

chx commented
Title: buildGrantsQueryCondition is not scope » buildGrantsQueryCondition does not have a scope
chx’s picture

chx commented
Title: buildGrantsQueryCondition does not have a scope » buildGrantsQueryCondition is static and does not have a scope
StatusFileSize
new 2401959_3.patch1.89 KB

Of course one can raise the question: why on earth is this static in the first place??

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Seems totally legit, this is an internal implementation detail of the storage.

xjm’s picture

xjm
she/her
Primary language English
commented
Assigned: xjm » Unassigned

+1.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Quickfix

Let's add a tag.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: buildGrantsQueryCondition is static and does not have a scope » buildGrantsQueryCondition() does not have a scope
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2401959-7.patch760 bytes

Discussed with chx in IRC; the problem is not static, but only the lack of scope.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

The answer to #3 and this needs to spread wider: static is preferred to indicate (and enforce) a function which doesn't change the object state. static got a bad rep because static variables are problematic but static methods are helpful in this way.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 3bd3baf and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed 3bd3baf on 8.0.x 
    Issue #2401959 by chx, Wim Leers: buildGrantsQueryCondition() does not...

Status: Fixed » Closed (fixed)

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