Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
In \Drupal\error_test\Controller\ErrorTestController::generateWarnings() we do
$bananas = [];
$monkey_love = $bananas[1];
To generate a notice - this produces a warning PHP 8.0.
Steps to reproduce
Proposed resolution
Move to code that produces a notice in all situations.
$notice = new \stdClass();
$notice == 1 ? 1 : 0;
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
None
Comments
Comment #2
alexpottComment #3
andypostMaybe it should just throw expectable warning instead of to be language specific?
Comment #4
alexpott@andypost this has to be a language level warning. You can't throw that.
Comment #5
dawehnerReading up on the RFC:
The object to scalar comparison is one of the ones which keep being a notice.
Comment #6
dawehnerAs a comparison this is what symfony does in its error handling: https://github.com/symfony/error-handler/blob/5.x/Tests/ErrorHandlerTest...
Comment #7
andypostMoreover SF testing both versions https://github.com/symfony/error-handler/blob/5.x/Tests/ErrorHandlerTest...
Comment #8
alexpott@dawehner @andypost we have a warning for PHP 7 & 8 already... we need something that is a notice on both. I don't understand why we need to copy symfony here.
Comment #9
andypostThis is critical blocker, so should be ready as our controller should provide language level warning as Symfony tests do more precise testing of specifics
Comment #10
alexpottRe
The purpose of \Drupal\error_test\Controller\ErrorTestController::generateWarnings() is to generate a PHP notice, PHP warning and PHP user error - to test how our error handler responds in those cases. Adding something like https://github.com/symfony/error-handler/blob/5.x/Tests/ErrorHandlerTest... to our test would mean that we'd lose PHP notice coverage in PHP 8 which is exactly what this issue is trying to (and successfully) avoiding.
Comment #11
andypostI was more about why this test is functional but Alex reminded me about mess in error handlers and we should do real requests (instead of kernel/unit test as SF doing), also we using exceptions in core instead of "goto" so there's issues like #2638686: Exception in EarlyRenderingControllerWrapperSubscriber is a DX nightmare, remove it
SO this test should remain functional with the same error message on php 7 and 8
Comment #15
xjmI think this is an acceptable fix for this, although I think we should add code comments documenting what's needed a little better (to reflect the discussion on this issue). That can be a followup, though.
As a test change, this is eligible for backport including to the production branch, and of course as a PHP 8 blocker it's a priority during the 9.1 alpha.
Committed to 9.2.x, 9.1.x, and 9.0.x. I didn't backport it to 8.9.x yet because I want to run tests against that first. Thanks!
Comment #16
xjmDoesn't apply in any case, so if we did want to backport this to 8.9.x, we'd need a separate patch.
Comment #17
andypostto backport it needs to backport previous fix #3174158: Division by zero is not warning since PHP 8.0 beta4
Comment #18
alexpottGiven the agreement on #3156651: Prevent Drupal 8.9 and 9.0 from being installed on PHP 8 this won't need a backport at this point.