Problem/Motivation

Just a quick fix for one of the baseline errors found by phpstan.

Steps to reproduce

Its in the baseline If you review the class you'll see the class isn't included. This doesn't currently throw any errors or warnings because the code isn't actually used.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#16 interdiff_14-16.txt762 bytesmrinalini9
#16 3264145-16.patch1.25 KBmrinalini9
#14 reroll_diff_3264145_7-14.txt1.45 KBankithashetty
#14 3264145-14.patch1.28 KBankithashetty
#7 interdiff_5-7.txt829 bytesravi.shankar
#7 3264145-7.patch1.27 KBravi.shankar
#5 reroll_diff_3264145-1_3264145-5.txt1.21 KByogeshmpawar
#5 3264145-5.patch1.21 KByogeshmpawar
#2 3264145-1.patch1.28 KBneclimdul

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Active » Needs review
StatusFileSize
new 3264145-1.patch1.28 KB
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Fixes a reported PHPStan error

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Needs a re-roll, also not sure why we wouldn't remove the unexecuted code - could use a follow-up for that at least.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Needs review
StatusFileSize
new 3264145-5.patch1.21 KB
new reroll_diff_3264145-1_3264145-5.txt1.21 KB

Updated patch will reroll diff added.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work

Per #4, instead of adding the use, we should be removing the code that is using the constant that must be dead if no error ensues. Or add a test if it is not dead.

ravi.shankar’s picture

ravi.shankar commented
StatusFileSize
new 3264145-7.patch1.27 KB
new interdiff_5-7.txt829 bytes

Tried to address comment #6, please review.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Re #4/#6 The issue is that the method belongs to the parent interface and the test class must implement it, but the method isn't triggered by the test so the runtime error never occurs.

I think we should just add the missing use statement and move on, rather than forcing a test case for a method that does nothing except return NULL anyway; PHPStan has already caught the problem and will catch it again if we reintroduce the bug.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review

Yeah, #5 still applies and is the correct fix as the failure in #7 shows so moving back to NR.

Expanding on longwave's comment, its a mock so we wouldn't test it anyways. We shouldn't test a test, just fix it. :-D

Maybe a dive into the relevant line would provide some clarity to reviewers and committers about why its currently "working" and why #7 broke the change broke things.

 public function createConfigObject($name, $collection = StorageInterface::DEFAULT_COLLECTION) {

The code parses cleanly now because a reference to an undefined class like this isn't a fatal error until they're referenced.

Example, the second call errors:


class foo {
    public static function bar($arg = DNE::Constant) {
        echo $arg;
    }
}

foo::bar("This works");
foo::bar();

Output:

This works
Fatal error: Uncaught Error: Class 'DNE' not found in /tmp/preview:4
Stack trace:
#0 /tmp/preview(10): foo::bar()
#1 {main}
  thrown in /tmp/preview on line 4

The call to createConfigObject provides the collection argument (all core calls seem to) so the default value is never used and the code "works". However changing a optional parameter like in #7 makes the method incompatible so it _does_ cause an error. Or an error in php 8 and a warning in php 7 because php is weird but it's all the same since it won't pass for us. PHPstan here is just warning us "hey if you ever called this wrong it would break" and its right and we can just fix it for correctness.

Final note, technically I guess we could replace the optional value with null and it would fix the warning and be correct. However, I don't think that makes as much since as matching the parent signature entirely even if the method always returns null.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Thank you for the explanations.

The last submitted patch, 5: 3264145-5.patch, failed testing. View results

The last submitted patch, 5: 3264145-5.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3264145-14.patch1.28 KB
new reroll_diff_3264145_7-14.txt1.45 KB

Rerolled the patch in #7. Also replaced the optional value with NULL in createConfigObject as suggested in #9.

Thanks!

daffie’s picture

daffie commented
Status: Needs review » Needs work 
+++ b/core/modules/config/tests/config_override_test/src/ConfigOverrider.php
@@ -36,7 +36,7 @@ public function getCacheSuffix() {
-  public function createConfigObject($name, $collection = StorageInterface::DEFAULT_COLLECTION) {
+  public function createConfigObject($name, $collection = NULL) {

This is the wrong change. The warning can be fixed by adding the use-statement use Drupal\Core\Config\StorageInterface;. Now is the StorageInterface not know, it is not defined in the directory of the test. Just like comment #8 is saying.

mrinalini9’s picture

mrinalini9 commented
Status: Needs work » Needs review
StatusFileSize
new 3264145-16.patch1.25 KB
new interdiff_14-16.txt762 bytes

Hi,

I have added the patch by addressing the suggestion in #15, please review it.

Thanks & Regards,
Mrinalini

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Thank you.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 10.0.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 5253862 and pushed to 10.1.x. Thanks!
Committed ad66289 and pushed to 10.0.x. Thanks!
Committed 5f5ea22 and pushed to 9.5.x. Thanks!
Committed 86f87e9 and pushed to 9.4.x. Thanks!

  • alexpott committed 5253862 on 10.1.x 
    Issue #3264145 by ankithashetty, yogeshmpawar, ravi.shankar, mrinalini9...

  • alexpott committed ad66289 on 10.0.x 
    Issue #3264145 by ankithashetty, yogeshmpawar, ravi.shankar, mrinalini9...

  • alexpott committed 5f5ea22 on 9.5.x 
    Issue #3264145 by ankithashetty, yogeshmpawar, ravi.shankar, mrinalini9...

  • alexpott committed 86f87e9 on 9.4.x 
    Issue #3264145 by ankithashetty, yogeshmpawar, ravi.shankar, mrinalini9...

Status: Fixed » Closed (fixed)

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