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.

Sign up for GitHub

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

Jump to bottom

[REF][PHP8.1] Fix a couple of deprecations in php8.1 by specifying th… #23851

Merged
merged 1 commit into from
Jul 9, 2022
Merged

[REF][PHP8.1] Fix a couple of deprecations in php8.1 by specifying th… #23851

merged 1 commit into from
Jul 9, 2022

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Jun 21, 2022 •
edited
Loading

…at return type may change in BaseParamQuery and by ensuring that we don't pass null into function_exists in I18n class

Overview

This fixes 2 deprecations in php8.1

  1. Deprecation notices like
Deprecated: Return type of CRM_Utils_SQL_BaseParamQuery::offsetUnset($offset) should either be compatible with ArrayAccess::offsetUnset(mixed $offset): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /home/jenkins/bknix-edge/build/build-1/web/sites/all/modules/civicrm/CRM/Utils/SQL/BaseParamQuery.php on line 237
  1. Deprecation notice Deprecated: function_exists(): Passing null to parameter #1 ($function) of type string is deprecated in /home/jenkins/bknix-edge/build/build-1/web/sites/all/modules/civicrm/CRM/Core/I18n.php on line 804

Before

Notices shown during boot to run tests

After

Less notices

ping @totten @eileenmcnaughton @demeritcowboy

@civibot
Copy link

civibot bot commented Jun 21, 2022

(Standard links)

@civibot civibot bot added the master label Jun 21, 2022
eileenmcnaughton
eileenmcnaughton reviewed Jun 21, 2022
View reviewed changes
CRM/Utils/SQL/BaseParamQuery.php Outdated
@@ -184,6 +184,7 @@ public function param($keys, $value = NULL) {
*
* @return bool
*/
#[ReturnTypeWillChange]
Copy link
Contributor

Choose a reason for hiding this comment

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

that is super interesting - it does feel like this particular one should be reliably a bool though?

eileenmcnaughton
eileenmcnaughton reviewed Jun 21, 2022
View reviewed changes
CRM/Utils/SQL/BaseParamQuery.php
@@ -202,6 +203,7 @@ public function offsetExists($offset) {
* @see param()
* @see ArrayAccess::offsetGet
*/
#[ReturnTypeWillChange]
Copy link
Contributor

Choose a reason for hiding this comment

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

does changing * @return mixed to * @return null|int make this unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so i think what it wants is a function hint of :mixed but that has different connotations in php7.x to php8.x

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't think there's any other way to do this to support both php 7 and 8, since mixed doesn't exist in php 7 (it thinks it's a class type and so then wants an instance of class mixed).

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing to think about, but maybe there's no way around it, is that it won't be possible to support both php 7 and 9 at the same time, because php 9 will ignore these #[ReturnTypeWillChange] and require a return type specified, but php 7 won't let you specify mixed. One possibility, but is a lot of work, is to refactor all these mixed functions to not be so vague. But I don't think civi will be the only project in that boat, so hopefully php 9 will be far enough out that civi will be able to stop supporting php 7 by then. It just might cause difficulty while trying to prep for php 9.

demeritcowboy
demeritcowboy reviewed Jun 30, 2022
View reviewed changes
CRM/Core/I18n.php Outdated
@@ -801,7 +801,7 @@ function ts($text, $params = []) {
if ($bootstrapReady) {
// just got ready: determine whether there is a working custom translation function
$config = CRM_Core_Config::singleton();
if (isset($config->customTranslateFunction) and function_exists($config->customTranslateFunction)) {
if (isset($config->customTranslateFunction) && !CRM_Utils_System::isNull($config->customTranslateFunction) && function_exists($config->customTranslateFunction)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the function name could be 0, but since that's unlikely could this just be if (!empty($config->customTranslateFunction) && function_exists($config->customTranslateFunction)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok have updated to that @demeritcowboy

demeritcowboy
demeritcowboy reviewed Jul 5, 2022
View reviewed changes
CRM/Core/I18n.php Outdated
@@ -801,7 +801,7 @@ function ts($text, $params = []) {
if ($bootstrapReady) {
// just got ready: determine whether there is a working custom translation function
$config = CRM_Core_Config::singleton();
if (isset($config->customTranslateFunction) and function_exists($config->customTranslateFunction)) {
if (isset($config->customTranslateFunction) && !empty($config->customTranslateFunction) && function_exists($config->customTranslateFunction)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I meant the whole thing would just be if (!empty($config->customTranslateFunction) && function_exists($config->customTranslateFunction)) { since there's no need to check isset if checking empty, but ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok updated to that now @demeritcowboy

@seamuslee001
[REF][PHP8.1] Fix a couple of deprecations in php8.1 by specifying th…
fc651f9 
…at return type may change in BaseParamQuery and by ensuring that we don't pass null into function_exists in I18n class

Replace some ReturnTypeWillChange with later return types as they should be backward compatible

Dave D Fix

Completely switch to just not empty on the customTranslationFunction check
@demeritcowboy demeritcowboy merged commit 7500074 into civicrm:master Jul 9, 2022
@eileenmcnaughton eileenmcnaughton deleted the php81_deprecations branch July 9, 2022 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eileenmcnaughton eileenmcnaughton eileenmcnaughton left review comments

@demeritcowboy demeritcowboy demeritcowboy left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seamuslee001 @eileenmcnaughton @demeritcowboy