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

[REF] Fix handling of NULL values in count_characters smarty modifier… #23318

Merged
merged 2 commits into from
May 26, 2022

Conversation

seamuslee001
Copy link
Contributor

… by creating a CiviCRM specific one that handles them

Overview

This fixes a PHP8.1 issue whetre passing NULL to the 2nd parameter to preg_match_all is deprecated

Before

Deprecation error on php8.1

Deprecated: preg_match_all(): Passing null to parameter #2 ($subject) of type string is deprecated in /home/jenkins/bknix-edge/build/build-2/web/sites/all/modules/civicrm/packages/Smarty/plugins/modifier.count_characters.php on line 27

Call Stack:
    0.0004     427320   1. {main}() /home/jenkins/bknix-edge/build/build-2/web/sites/all/modules/civicrm/xml/GenCode.php:0
    0.0048    1094120   2. CRM_Core_CodeGen_Main->main() /home/jenkins/bknix-edge/build/build-2/web/sites/all/modules/civicrm/xml/GenCode.php:58
    0.2600    9389072   3. CRM_Core_CodeGen_Schema->run() /home/jenkins/bknix-edge/build/build-2/web/sites/all/modules/civicrm/CRM/Core/CodeGen/Main.php:115

After

no deprecation error

ping @eileenmcnaughton @demeritcowboy

… by creating a CiviCRM specific one that handles them
@civibot
Copy link

civibot bot commented Apr 28, 2022

(Standard links)

@civibot civibot bot added the master label Apr 28, 2022
@eileenmcnaughton
Copy link
Contributor

@seamuslee001

image

@totten
Copy link
Member

totten commented May 10, 2022

@seamuslee001 I added a commit to fixup the code-style issue.

I couldn't figure out how to reproduce a warning from the snippet in schema.tpl. Tried GENCODE_FORCE=1 and tried deleting/regenerating various files (sql/civicrm.mysql and most DAOs). FWIW, I'm running php 8.1.2.

Did you originally observe the error in GenCode -- or was that just an incidental bit that up during search/replace? (I think it's fine to change that one proactively -- just not clear but what to test.)

@demeritcowboy
Copy link
Contributor

demeritcowboy commented May 11, 2022

I see the schema one is doing something - before the patch if I do

cd xml
php GenCode.php

Then I get a bunch of errors about line 27: PHP Deprecated: preg_match_all(): Passing null to parameter #2 ($subject) of type string is deprecated in ...\packages\Smarty\plugins\modifier.count_characters.php on line 27

Am also using php 8.1.2.

If I only apply the schema part of the patch I don't get those, so it must be from there.

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label May 11, 2022

if (is_null($string)) {
return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is mergeable as-is just noting some possible improvements:

  • The check for null here could come before potentially passing to strlen()
  • While copied from the original function, strlen isn't multibyte-friendly. In the places where it's used currently it doesn't make too much difference.
  • The use in schema.xml is really checking for something like if $field.default !== null && $field.default !== '' (i.e. '0' is allowed but not null or blank). So could avoid the function.

@demeritcowboy
Copy link
Contributor

Going to merge but I'm going to put up a followup to move the check for null to ahead of sending it to strlen.

@demeritcowboy demeritcowboy merged commit 8d800ae into civicrm:master May 26, 2022
@eileenmcnaughton eileenmcnaughton deleted the php81_preg_replace branch May 26, 2022 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants