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 issue where setLogger did not adhere to the LoggerAwareInterface and change minimum PHP requirement to PHP 8 #286

Merged
merged 34 commits into from
Apr 13, 2023

Conversation

Rocksheep
Copy link
Contributor

@Rocksheep Rocksheep commented Oct 14, 2022

The setLogger method on the Api class never adhered to the setLogger method of LoggerAwareInterface. This is causing issues when using a newer version of the psr/log libraries which uses return types.

While testing I noticed that there was a variable being used that was not declared, so I swapped it out with false. Let me know if this is alright with you guys.

This PR fixes #273

@cla-bot
Copy link

cla-bot bot commented Oct 14, 2022

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 @Rocksheep.

@Rocksheep
Copy link
Contributor Author

The CLA is signed!

@RCheesley
Copy link
Member

@cla-bot check

Thanks for the PR @Rocksheep and welcome to the community!

@cla-bot cla-bot bot added the cla-signed label Oct 14, 2022
@cla-bot
Copy link

cla-bot bot commented Oct 14, 2022

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

@RCheesley RCheesley requested a review from escopecz October 14, 2022 13:26
@Rocksheep
Copy link
Contributor Author

I have updated the minimum PHP version to 8.0 for now and PHP CS fixer has been updated as well. It was the only dependency I found that really needed an update.

@Rocksheep Rocksheep changed the title Fix issue where setLogger did not adhere to the LoggerAwareInterface Fix issue where setLogger did not adhere to the LoggerAwareInterface and change minimum PHP requirement to PHP 8 Oct 20, 2022
@JaZo JaZo mentioned this pull request Oct 21, 2022
2 tasks
@hugronaphor
Copy link

So, since php 7.4 is end of life. Should a new release be made to include this change?

@escopecz
Copy link
Member

escopecz commented Jan 3, 2023

Yes, since we plan to release a new major version, we can ditch PHP 7.4 support.

* Use PHP 8.0

* Bump actions/checkout to v3

* Run composer update

* Bump php-cs-fixer for PHP 8.0 support

* Fixing failing tests due to new duplicity checks in Mautic

Co-authored-by: John Linhart <[email protected]>
@ariccadonna
Copy link

Hey there, any news regarding this PR?
Will it be merged or we have to wait for the next major?
We actually need to update our prod env and we're stuck because of this issue.

Thanks

@christian-beckmann
Copy link

christian-beckmann commented Feb 10, 2023

To use the fork, just add this to your composer.json

"repositories": [
    {
        "type": "vcs",
        "url": "https://github.com/Rocksheep/api-library.git"
    }
],
"require": {
    "mautic/api-library": "dev-upgrade-to-php8",
}

@RCheesley
Copy link
Member

As far as I can tell, nobody has tested or reviewed this PR, and it won't be merged until that happens.

Maybe you can join us with our Open Source Friday sprints in #t-product on slack, that's a great way to encourage people to test PRs.

Testing instructions are also super helpful.

More on testing: https://contribute.mautic.org/contributing-to-mautic/tester

On the requirements to merge: https://contribute.mautic.org/community-structure/governance/code-governance

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
@Rocksheep
Copy link
Contributor Author

Alright, the branch has been synced and the conflicts have been resolved

@RCheesley
Copy link
Member

@all-contributors please add @Rocksheep for code

@allcontributors
Copy link
Contributor

@RCheesley

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

@RCheesley
Copy link
Member

@all-contributors please add @PierreAmmeloot for userTesting

@allcontributors
Copy link
Contributor

@RCheesley

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

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.

I see no issue with these changes 👍

@escopecz
Copy link
Member

Although the CI might need more work after the CS Fixer update...

@Rocksheep
Copy link
Contributor Author

I'll fix the CS script and the resulting issues. So give me a few minutes and then it should all work! It's mostly some untyped @params in the docs and public constants that don't have the public keyword.

@Rocksheep
Copy link
Contributor Author

All the style changes have been fixed and the action should be configured correctly now.

@codecov-commenter
Copy link

Codecov Report

Merging #286 (f0b7b0c) into main (494807f) will increase coverage by 2.10%.
The diff coverage is 14.28%.

❗ Current head f0b7b0c differs from pull request most recent head 0995283. Consider uploading reports for the commit 0995283 to get more accurate results

@@             Coverage Diff              @@
##               main     #286      +/-   ##
============================================
+ Coverage     49.94%   52.05%   +2.10%     
+ Complexity      408      407       -1     
============================================
  Files            30       30              
  Lines           973     1093     +120     
============================================
+ Hits            486      569      +83     
- Misses          487      524      +37     
Impacted Files Coverage Δ
lib/Api/Api.php 79.87% <0.00%> (+1.64%) ⬆️
lib/Api/CampaignEvents.php 58.33% <ø> (-11.67%) ⬇️
lib/Api/Contacts.php 94.04% <ø> (+3.66%) ⬆️
lib/Auth/BasicAuth.php 100.00% <ø> (ø)
lib/Auth/OAuth.php 0.00% <0.00%> (ø)
lib/Exception/AbstractApiException.php 100.00% <ø> (ø)
...ib/Exception/UnexpectedResponseFormatException.php 100.00% <ø> (ø)
lib/QueryBuilder/QueryBuilder.php 50.00% <ø> (-4.06%) ⬇️
lib/QueryBuilder/WhereBuilder.php 90.58% <ø> (+0.58%) ⬆️
lib/Auth/AbstractAuth.php 74.73% <100.00%> (+1.40%) ⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@escopecz escopecz merged commit 6bd0d47 into mautic:main Apr 13, 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.

Upgrade from 3.0.0 to 3.1.0 issue with psr/log requirement
10 participants