-
-
Notifications
You must be signed in to change notification settings - Fork 824
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 PHP8.1 issue with passing NULL to trim() #23317
[REF] Fix PHP8.1 issue with passing NULL to trim() #23317
Conversation
(Standard links)
|
CRM/Core/CodeGen/Specification.php
Outdated
'idColumn' => trim($foreignXML->idColumn), | ||
'typeColumn' => trim($foreignXML->typeColumn), | ||
'key' => trim($this->value('key', $foreignXML)), | ||
'idColumn' => trim($foreignXML->idColumn ?? ''), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this PR is another test just to trigger the test system but I remember simplexml objects don't behave like normal. This would never be null unless $foreignXML itself is null because idColumn itself is an object even if it doesn't exist in the original xml. I think in the end it's ok, but I don't know if it's catching the situation you're trying to catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was mostly key that was the problem but I might have just been a bit overzealos with the protection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you want to do with this one? If it is just "key", then I'd lean towards something narrower so as not to hide real errors. Like just replacing that line with 'key' => trim($this->value('key', $foreignXML, '')),
4950846
to
74ba6b1
Compare
Filled in the Regarding the discussion about Force-pushed to make the patch a little smaller. Merge on pass. |
Overview
Fix a console warning when running
setup.sh
on php81.To reproduce, you can use these commands:
Before
After
This warning is resolved.
Comments
There are still other issues remaining for php81 compatibility (e.g.
strftime
in Smarty). However, this gets us closer.