Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MNT Handle DatabaseExceptions thrown by PHP 8.1 #311

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Jul 18, 2022

PHP 8.1 set the MySQLi default error mode to exceptions https://php.watch/versions/8.1/mysqli-error-mode

This resulted in https://github.com/silverstripe/cwp/runs/7351123074?check_suite_focus=true

It would have been nice to just add | DatabaseException to

} catch (InvalidArgumentException $e) {
as talked about here

However the failing FunctionalTest (which makes an HTTP request to the frontend) simply doesn't go through this bit of code.

Incorrect DATETIME value: 'christmas 00:00:00'
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/ORM/Connect/DBConnector.php:64
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/ORM/Connect/MySQLiConnector.php:71
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/ORM/Connect/MySQLiConnector.php:289
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/ORM/Connect/Database.php:185
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/ORM/Connect/Database.php:258
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/ORM/Connect/Database.php:183
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/ORM/Connect/MySQLDatabase.php:402
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/ORM/DB.php:445
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/ORM/Queries/SQLExpression.php:115
/home/runner/work/cwp/cwp/src/PageTypes/DatedUpdateHolder.php:201
/home/runner/work/cwp/cwp/src/PageTypes/DatedUpdateHolderController.php:289
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/ViewableData.php:485
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/ViewableData.php:532
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/SSViewer_Scope.php:323
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/SSViewer_DataPresenter.php:309
/tmp/silverstripe-cache-php8.1.7-home-runner-work-cwp-cwp/runner/.cachethemes.starter.templates.CWP.CWP.PageTypes.Layout.EventHolder.ss:46
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/SSViewer.php:602
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/SSViewer.php:674
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/SSViewer.php:668
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/SSViewer_DataPresenter.php:330
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/SSViewer_DataPresenter.php:363
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/SSViewer_DataPresenter.php:165
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/SSViewer_DataPresenter.php:296
/tmp/silverstripe-cache-php8.1.7-home-runner-work-cwp-cwp/runner/.cachethemes.starter.templates.Page.ss:72
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/SSViewer.php:602
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/View/SSViewer.php:674
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Controller.php:302
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/RequestHandler.php:202
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Controller.php:212
/home/runner/work/cwp/cwp/vendor/silverstripe/cms/code/Controllers/ContentController.php:251
/home/runner/work/cwp/cwp/vendor/silverstripe/cms/code/Controllers/ModelAsController.php:101
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Director.php:360
/home/runner/work/cwp/cwp/vendor/silverstripe/versioned/src/VersionedHTTPMiddleware.php:41
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/session-manager/src/Middleware/LoginSessionMiddleware.php:29
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/ConfirmationMiddleware.php:254
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/ConfirmationMiddleware.php:254
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Security/PasswordExpirationMiddleware.php:84
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Security/BasicAuthMiddleware.php:68
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Security/AuthenticationMiddleware.php:61
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/ExecMetricMiddleware.php:20
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/CanonicalURLMiddleware.php:190
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPCacheControlMiddleware.php:42
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/ChangeDetectionMiddleware.php:28
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/FlushMiddleware.php:27
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/RequestProcessor.php:66
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/hybridsessions/src/Control/HybridSessionMiddleware.php:18
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/AllowedHostsMiddleware.php:60
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/TrustedProxyMiddleware.php:176
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/cwp/cwp-core/src/Control/InitialisationMiddleware.php:89
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:62
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Middleware/HTTPMiddlewareAware.php:65
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Director.php:369
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Director.php:134
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Director.php:279
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Control/Director.php:133
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Dev/TestSession.php:107
/home/runner/work/cwp/cwp/vendor/silverstripe/framework/src/Dev/FunctionalTest.php:183
/home/runner/work/cwp/cwp/tests/PageTypes/DatedUpdateHolderControllerTest.php:59
/home/runner/work/cwp/cwp/vendor/bin/phpunit:123

@GuySartorelli
Copy link
Member

I don't think this actually solves the problem - users are going to get different behaviour depending on what database version is used which is not good.
I didn't realise you had already created this PR and was working on the same issue from the recipe-solr-search broken build and created this PR: #312
I think it's better to fix the problem and always return the same error to the user rather than allowing the exception to throw for mysql8.0

@emteknetnz emteknetnz force-pushed the pulls/2.9/broken-builds branch 11 times, most recently from dddcdd6 to 248faa7 Compare July 18, 2022 06:44
@emteknetnz
Copy link
Member Author

Changed solution, this matches behavior to PHP < 8.1

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds some extra DB queries which isn't great but since you've closed my PR I guess we'll have to go with this approach lol
One small thing to fix.

strpos($e->getMessage(), 'Incorrect DATETIME value') !== false
) {
$controller->getRequest()->getSession()->set(DatedUpdateHolderController::TEMP_FORM_MESSAGE, _t(
__CLASS__ . '.InvalidDateFormat',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
__CLASS__ . '.InvalidDateFormat',
DatedUpdateHolderController::class . '.InvalidDateFormat',

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@emteknetnz emteknetnz force-pushed the pulls/2.9/broken-builds branch from 248faa7 to d14cf42 Compare July 18, 2022 21:41
@GuySartorelli GuySartorelli merged commit 35b7f70 into silverstripe:2.9 Jul 19, 2022
@GuySartorelli GuySartorelli deleted the pulls/2.9/broken-builds branch July 19, 2022 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants