Problem/Motivation

The advisory_feed_test module extends and decorates the guzzle client to rewrite the request url during testing. This is a problem though because the guzzle client is marked @final so could break including possibly and actually fatal error if future versions are really final.

PHPStan correctly marks this as a problem so its in our baseline for level 0 fixes.

Steps to reproduce

This is an interaction with an annotation so it isn't causing errors but it does fix an entry in the phpstan baseline.

Proposed resolution

Replace the extended class with Middleware that can accomplish the same result.

CommentFileSizeAuthor
#18 3264167-18.patch8.64 KBspokje
#18 interdiff.15-18.txt878 bytesspokje
#15 3264167-15.patch8.71 KBspokje
#15 raw_diff.14-15.txt1.18 KBspokje
#14 3264167-14.interdiff.txt880 bytesneclimdul
#14 3264167-14.patch8.83 KBneclimdul
#10 interdiff_3264167-1-10.txt836 bytesheykarthikwithu
#10 3264167-10.patch8.83 KBheykarthikwithu
#2 3264167-1.patch8.67 KBneclimdul

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
commented
Component: settings_tray.module » system.module
Status: Active » Needs review
StatusFileSize
new 3264167-1.patch8.67 KB

This achieved the same result locally and fixed the reported error.

Status: Needs review » Needs work

The last submitted patch, 2: 3264167-1.patch, failed testing. View results

neclimdul’s picture

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

This doesn't really make sense, sounds like a random failure. Re-queueing.

Quickedit.Drupal\Tests\quickedit\FunctionalJavascript\CKEditor5IntegrationTest
	✗	
Drupal\Tests\quickedit\FunctionalJavascript\CKEditor5IntegrationTest

fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-139.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.11 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\quickedit\FunctionalJavascript\CKEditor5IntegrationTest
F.                                                                  2 / 2 (100%)

Time: 00:58.790, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\quickedit\FunctionalJavascript\CKEditor5IntegrationTest::testDiscard
Field node/1/body/en/full did not match its expectation selector (.quickedit-field.quickedit-editable.quickedit-candidate.quickedit-highlighted.quickedit-editing.quickedit-changed)
Failed asserting that a NULL is not empty.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditJavascriptTestBase.php:267
/var/www/html/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditJavascriptTestBase.php:244
/var/www/html/core/modules/quickedit/tests/src/FunctionalJavascript/CKEditor5IntegrationTest.php:172
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

Status: Needs review » Needs work

The last submitted patch, 2: 3264167-1.patch, failed testing. View results

neclimdul’s picture

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

lots of random failures... yikes the bot was not happy last night.

longwave’s picture

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

Nice fix, Guzzle middleware is the more correct way of intercepting and modifying outbound HTTP calls, so +1 from me.

longwave’s picture

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

Actually, two minor nits:

  1. +++ b/core/modules/system/tests/modules/advisory_feed_test/src/AdvisoryTestClientMiddleware.php
@@ -0,0 +1,42 @@
+class AdvisoryTestClientMiddleware {

    Missing class docblock. 

  2. +++ b/core/modules/system/tests/modules/advisory_feed_test/src/AdvisoryTestClientMiddleware.php
@@ -0,0 +1,42 @@
+  public function __invoke() {

    Missing method docblock.

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
Assigned: Unassigned » heykarthikwithu
heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3264167-10.patch8.83 KB
new interdiff_3264167-1-10.txt836 bytes

Added missing docblock's

spokje’s picture

spokje
Primary language Dutch
commented
Title: Remove use of final Guzzle client class » [PP-1] Remove use of final Guzzle client class
Status: Needs review » Postponed
Related issues: +#3266535: Remove remaining references to the hal module from core/phpstan-baseline.neon

Postponing on (at least) removing some hal ghost, to prevent PHPStan to block the patch here: #3266535: Remove remaining references to the hal module from core/phpstan-baseline.neon.

spokje’s picture

spokje
Primary language Dutch
commented
Title: [PP-1] Remove use of final Guzzle client class » Remove use of final Guzzle client class
Status: Postponed » Needs review

#3266535: Remove remaining references to the hal module from core/phpstan-baseline.neon was committed already (on the phone with the Guinness Book of World Records as we speak to confirm record speed), unpostponing and restarting a new test on the latest patch.

Any "Custom Commands Failed" that will return are now directly related to that patch

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Needs work
neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review
StatusFileSize
new 3264167-14.patch8.83 KB
new 3264167-14.interdiff.txt880 bytes

touch up the docs just a tiny bit and adding though full stops. Thanks

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new raw_diff.14-15.txt1.18 KB
new 3264167-15.patch8.71 KB
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented 
+++ b/core/modules/system/tests/modules/advisory_feed_test/src/AdvisoryTestClientMiddleware.php
@@ -0,0 +1,48 @@
+          $request->withUri(new Uri($test_end_point));
+          return $handler($request->withUri(new Uri($test_end_point)), $options);

Why do we call ->withUri() twice? Would these two lines work better as just

$request = $request->withUri(new Uri($test_end_point));

and then fall through to the default return?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.15-18.txt878 bytes
new 3264167-18.patch8.64 KB

I've only rerolled this one without have a decent look at it (Yep, I'll never learn...), but the above makes sense to me.

smustgrave’s picture

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

Ran against 10.1 tests and no issue. Seems like a good test refactor.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 3264167-18.patch, failed testing. View results

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Seems to be an unrelated failure.

rohan-sinha’s picture

rohan-sinha commented

Tested the Patch #18 on D10, specifically 10.1x , worked well , no issue addressed.

  • longwave committed fab2a835 on 10.0.x 
    Issue #3264167 by Spokje, neclimdul, heykarthikwithu, longwave: Remove...

  • longwave committed cc218774 on 10.1.x 
    Issue #3264167 by Spokje, neclimdul, heykarthikwithu, longwave: Remove...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Committed and pushed cc21877471 to 10.1.x and fab2a8350a to 10.0.x. Thanks!

longwave’s picture

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

Status: Fixed » Closed (fixed)

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