Problem/Motivation

We have some left behind get_called_class() and incorrect get_class($this) calls that there is a keyword static::class that has replaced the need for that call to get the late binding class.

@see https://secure.php.net/get_called_class

Proposed resolution

Replace get_called_class() and get_class($this) with static::class

Remaining tasks

TBD

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

TBD

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Title: Convert all get_called_class() to static:: » Convert all get_called_class()/get_class() to static::
Issue summary: View changes
Status: Active » Needs review
Issue tags: +API clean-up
FileSize
10.32 KB

There are some legit calls, I think I got all the outliers.

Status: Needs review » Needs work

The last submitted patch, 2: convert_all-2619482-2.patch, failed testing.

joelpittet’s picture

Priority: Normal » Minor
Status: Needs work » Needs review
Issue tags: -Performance
FileSize
1.52 KB

NM, render arrays... that's why fails. These may be the only 3 hold outs.

joelpittet’s picture

Status: Needs review » Closed (works as designed)

Nope, I'm wrong:) They are all correct.

Status: Closed (works as designed) » Needs work

The last submitted patch, 4: convert_all-2619482-4.patch, failed testing.

joelpittet’s picture

Status: Needs work » Closed (works as designed)

Yes I know testbot! chill out;)

cburschka’s picture

Version: 8.0.x-dev » 9.0.x-dev
Status: Closed (works as designed) » Active

Sorry if I misunderstand the purpose of this old issue. I was about to create one with basically the same subject, before looking it up. I think that the problem was (and is) valid; the approach just wasn't right.

We can't use the keyword 'static' inside a string, since it will not be resolved correctly. Instead of replacing [get_called_class(), 'method'] with 'static::method' the patch should have replaced it with static::class . '::method', which is indeed already used in a lot of places in core.

Per this StackOverflow answer, get_called_class() is fully identical to static::class (in PHP 5.5+), and the latter has a minor performance benefit.

in 9.0.x, get_called_class() is still used in 18 files (14 of them in the form [get_called_class(), '...'] ), while static::class is used in 51 files (11 of them in the form static::class . '::...').
Just for consistency (if negligible performance), we might want to look at this again.

cburschka’s picture

Version: 9.0.x-dev » 9.1.x-dev
Status: Active » Needs review
FileSize
21.86 KB
cburschka’s picture

Bad patch.

This one replaces get_called_class() with static::class everywhere.

A further replacement we might consider is replacing static::class . '::method' with [static::class, 'method'] everywhere. The two are semantically equivalent and the latter is used far more commonly in core.

cburschka’s picture

get_class($this) can also be replaced with static::class.

(There is no such replacement for get_class() in general.)

daffie’s picture

Talked to @alexpott on slack about this issue. He is ok with both changes: get_called_class() to static::class and get_class($this) to static::class.
The IS needs an update.

The changes in the patch look good. But when I do a code base search for "get_class($this)" I still get a 167 results after I applied the patch.

ankitsingh0188’s picture

Assigned: Unassigned » ankitsingh0188

I am working on this.

ankitsingh0188’s picture

Assigned: ankitsingh0188 » Unassigned
Status: Needs work » Needs review
FileSize
119.9 KB
102.72 KB
siddhant.bhosale’s picture

Assigned: Unassigned » siddhant.bhosale

Status: Needs review » Needs work

The last submitted patch, 14: 2619482-14.patch, failed testing. View results

cburschka’s picture

Status: Needs work » Needs review

The webdriver tests have always been a bit brittle, might be a random failure. Retesting.

siddhant.bhosale’s picture

Assigned: siddhant.bhosale » Unassigned
Status: Needs review » Reviewed & tested by the community

The patch is applied successfully and looks good to be merged.

daffie’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated the IS.

cburschka’s picture

Confirmed that all changes in the patch replace instances of 'get_called_class()/get_class($this) with static::class, and that there aren't any of them left after applying it.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a re-roll.

ravi.shankar’s picture

Assigned: Unassigned » ravi.shankar

Working on this.

ravi.shankar’s picture

Assigned: ravi.shankar » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
120.6 KB

Added reroll of patch #14.

jungle’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
18.18 KB
  1. Attaching a raw-interdiff.txt file.
  2. $ grep -r get_called_class core composer --exclude-dir=core/node_modules
    [no output]
    $ grep -r 'get_class($this)' core composer --exclude-dir=core/node_modules
    [no output]
    

    No leftovers.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed eafefd8 and pushed to 9.1.x. Thanks!

  • catch committed eafefd8 on 9.1.x
    Issue #2619482 by cburschka, ankit.singh, ravi.shankar, jungle, daffie:...

Status: Fixed » Closed (fixed)

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