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

[php8-compat] Update smarty templates and some php files to get the a… #20512

Merged

Conversation

seamuslee001
Copy link
Contributor

…pi_v3_contribution testclass to pass on php8

Overview

This makes necessary changes to get the test class to pass on php8

Before

Tests fail php8

After

Tests pass on php8

Technical Details

Most if not all the changes are primarily focused on dealing with issues where a smarty variable isn't assigned to the template which in php8 generates a harder array key doesn't exist error

ping @eileenmcnaughton @colemanw

@civibot
Copy link

civibot bot commented Jun 5, 2021

(Standard links)

@civibot civibot bot added the master label Jun 5, 2021
@seamuslee001 seamuslee001 force-pushed the php8_contribution_api_tests branch 3 times, most recently from 48161d1 to 090e794 Compare June 5, 2021 09:08
@eileenmcnaughton
Copy link
Contributor

I kinda wish we didn't have to do this as these are probably all points of potential leakage & ensuring they are assigned would be better - but I accept that we shouldn't make that an obstacle to php8 support

@demeritcowboy
Copy link
Contributor

Just thinking out loud I wonder if it would be easier to have an error handler that just logs any errors generated by files under templates_c to a separate log channel, so they don't trigger test fails but still get logged and can be dealt with as time permits.

I expect there's going to be tons of these even beyond what comes up in tests - with the loudnotices extension I had to skip templates_c because pretty much every page in civi crashed.

@seamuslee001 seamuslee001 force-pushed the php8_contribution_api_tests branch from 090e794 to 40a6c1d Compare June 5, 2021 22:27
@eileenmcnaughton
Copy link
Contributor

@demeritcowboy yeah - I'm not sure why the 'turn notices off in smarty' doesn't work with php8 but I guess @seamuslee001 is trying to get the volume down enough than he can see the woods at this stage

…pi_v3_contribution testclass to pass on php8
@seamuslee001 seamuslee001 force-pushed the php8_contribution_api_tests branch from 40a6c1d to ff708ff Compare June 5, 2021 23:18
@seamuslee001
Copy link
Contributor Author

I think it likely partly this

The @ operator will no longer silence fatal errors (E_ERROR, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR, E_RECOVERABLE_ERROR, E_PARSE). Error handlers that expect error_reporting to be 0 when @ is used, should be adjusted to use a mask check instead:

In conjunction with

A number of warnings have been converted into Error exceptions:

Attempting to write to a property of a non-object. Previously this implicitly created an stdClass object for null, false and empty strings.
Attempting to append an element to an array for which the PHP_INT_MAX key is already used.
Attempting to use an invalid type (array or object) as an array key or string offset.
Attempting to write to an array index of a scalar value.
Attempting to unpack a non-array/Traversable.
Attempting to access unqualified constants which are undefined. Previously, unqualified constant accesses resulted in a warning and were interpreted as strings.

see https://www.php.net/manual/en/migration80.incompatible.php

@seamuslee001 seamuslee001 merged commit 233a5b0 into civicrm:master Jun 6, 2021
@seamuslee001 seamuslee001 deleted the php8_contribution_api_tests branch June 6, 2021 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants