Skip to content

Update components to be compatible with php82 - #239

Open
toanhminh0412 wants to merge 40 commits into
test-php82from
update-components-php82
Open

Update components to be compatible with php82#239
toanhminh0412 wants to merge 40 commits into
test-php82from
update-components-php82

Conversation

@toanhminh0412

@toanhminh0412 toanhminh0412 commented Oct 23, 2024

Copy link
Copy Markdown
Collaborator

This work was done by:

  • Creating a patch file by diffing the core php8 code and core php5 code and applying them onto the current app code
  • Going through the site page by page and fixing whatever error that comes up

@toanhminh0412 toanhminh0412 mentioned this pull request Oct 23, 2024
1 task
Avoid passing null to trim
Avoid passing null as the second param to preg_match_all
Avoid passing null to strtolower
Avoid passing null to strtolower
Avoid passing null to stripslashes
Avoid passing null to count
Avoid passing null to json_decode
Avoid passing null to stripslashes
Avoid passing null to stripslashes
Avoid passing null to stripslashes
->set('group', $group)
->set('customFields', $customFields)
->set('customAnswers', $customFieldForm)
->set('customAnswers', $customFieldForms)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I couldn't find where 'customFieldForms' is being created in the code. The change to pluralize 'form' to 'forms' here is intentional though?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No, good catch. I need to fix this at some point

@leenapthine

Copy link
Copy Markdown
Collaborator

Nice work Archie! I went through about half of the patch code, and then finished by going through all of your manual fixes where the patch needed tweaking. Everything looks good to me.

@toanhminh0412
toanhminh0412 requested review from dleske and leenapthine and removed request for leenapthine December 13, 2024 22:37
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