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

Fix - Deprecated PHP 8.1 #293

Conversation

LadySolveig
Copy link
Contributor

Fix for
Deprecated
: strlen(): Passing null to parameter #1 ($string) of type string is deprecated in api-library/lib/Auth/OAuth.php on

  • line 388
  • line 395
  • line 637

Fix for 
Deprecated
: strlen(): Passing null to parameter mautic#1 ($string) of type string is deprecated in api-library/lib/Auth/OAuth.php on 
- line 388
- line 395
- line 637
@cla-bot
Copy link

cla-bot bot commented Apr 10, 2023

Thank you for your contribution! We require all contributors to sign our Contributor License Agreement, and we do not have a record of your signature on file. In order for us to review and merge your code, please head over to https://www.mautic.org/contributor-agreement and complete the form. There may be a short delay while the team add you as a contributor - please be patient :). Any problems contact the Product Team on Slack (get an invite at https://mautic.org/slack). CLA has not been signed by @LadySolveig.

@LadySolveig
Copy link
Contributor Author

Thank you for your contribution! We require all contributors to sign our Contributor License Agreement, and we do not have a record of your signature on file. In order for us to review and merge your code, please head over to https://www.mautic.org/contributor-agreement and complete the form. There may be a short delay while the team add you as a contributor - please be patient :). Any problems contact the Product Team on Slack (get an invite at https://mautic.org/slack). CLA has not been signed by @LadySolveig.
Signed ✅

@RCheesley
Copy link
Member

@cla-bot check

Thanks for signing the CLA @LadySolveig and welcome to the Mautic community!

We have quite a bit of work to do on 8.1 support in Mautic, we're hoping to get it in for the 5.0 release. If you've got any capacity to help with that please do let us know!

@cla-bot cla-bot bot added the cla-signed label Apr 11, 2023
@cla-bot
Copy link

cla-bot bot commented Apr 11, 2023

The CLA Bot has been sent on a mission to check against the latest list and will be back shortly with its findings!

Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

The changes make sense. Thanks for taking care of this!

I think it could be simplified.

lib/Auth/OAuth.php Outdated Show resolved Hide resolved
lib/Auth/OAuth.php Outdated Show resolved Hide resolved
lib/Auth/OAuth.php Outdated Show resolved Hide resolved
@LadySolveig
Copy link
Contributor Author

The changes make sense. Thanks for taking care of this!

I think it could be simplified.

You're absolutely right. I saw the line 525 in code where it was just fixed with

return !empty($this->_request_token_url) && strlen($this->_request_token_url) > 0;

Perhaps this could be also only

return !empty($this->_request_token_url);

deleted strlen() completely from lines
Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you! We have the tests broken since last week as we are upgrading Mautic to Symofny 5. We'll fix that on the Mautic's side and re-run the tests to get the CI check green. Once it's green we can merge this PR.

@RCheesley
Copy link
Member

@all-contributors please add @LadySolveig for code

@allcontributors
Copy link
Contributor

@RCheesley

I've put up a pull request to add @LadySolveig! 🎉

@RCheesley
Copy link
Member

@all-contributors please add @escopecz for review

@allcontributors
Copy link
Contributor

@RCheesley

I've put up a pull request to add @escopecz! 🎉

@PierreAmmeloot
Copy link

Hello @RCheesley, I have check on my side.
It's OK.

Can you close and merge this fix?

Pierre

@RCheesley
Copy link
Member

@PierreAmmeloot please can you leave your review as detailed here so that it counts as a contribution and tells GitHub we have had a positive review?

Also as @escopecz mentioned we can't merge this until the tests are passing, which is pending some fixes being merged in mautic/mautic (I think they might have just got fixed - just re-running the tests).

If you have any time we would greatly appreciate some help testing and reviewing the PRs for the Symfony 5 project. Check out #t-product on Slack for more guidance from the team in that regard!

@escopecz
Copy link
Member

Yep, the CI is green. Merging.

@escopecz escopecz merged commit 3b96c2d into mautic:main Apr 12, 2023
@RCheesley RCheesley added this to the 4.0 milestone Jul 18, 2024
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.

4 participants