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/php8.1 depreciation #1861

Merged
merged 5 commits into from
Aug 19, 2022
Merged

Fix/php8.1 depreciation #1861

merged 5 commits into from
Aug 19, 2022

Conversation

Grotax
Copy link
Member

@Grotax Grotax commented Aug 6, 2022

This is supposed to fix all the deprication warnings of php 8.1 that I can find. It seems like mainly to be that you can't pass null to a method that expects a string. Old behaviour of these methods was to interpret null as "" which then is usually also the output.

This will change the api behavior same like the change for the author did. Even though it was never declared I think it is fair to put this into news 19.0.0 as major change.

To fix the docs gap I also checked which attributes of an item are exposed via the api sorted alphabetically and added default + type info. Most types default will probably be null. But I didn't want to put that because I wasn't sure.

@Grotax Grotax force-pushed the fix/php8.1_depreciation branch 2 times, most recently from 2c32fb8 to 77d9a39 Compare August 6, 2022 12:08
@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2022

Codecov Report

Merging #1861 (be4cf4e) into master (fca05d5) will decrease coverage by 0.13%.
The diff coverage is 89.70%.

@@             Coverage Diff              @@
##             master    #1861      +/-   ##
============================================
- Coverage     91.62%   91.49%   -0.14%     
- Complexity      778      793      +15     
============================================
  Files            65       65              
  Lines          2724     2776      +52     
============================================
+ Hits           2496     2540      +44     
- Misses          228      236       +8     
Impacted Files Coverage Δ
lib/Db/ItemMapperV2.php 98.51% <60.00%> (-1.49%) ⬇️
lib/Service/FeedServiceV2.php 97.80% <81.81%> (-2.20%) ⬇️
lib/Fetcher/FeedFetcher.php 80.62% <95.00%> (+0.89%) ⬆️
lib/Db/Item.php 100.00% <100.00%> (ø)
lib/Utility/OPMLExporter.php 100.00% <100.00%> (ø)
lib/Command/ExploreGenerator.php 100.00% <0.00%> (ø)
lib/Explore/RecommendedSites.php 0.00% <0.00%> (ø)
lib/Controller/FeedController.php 100.00% <0.00%> (ø)
lib/Controller/PageController.php 100.00% <0.00%> (ø)
lib/Fetcher/Client/FeedIoClient.php 100.00% <0.00%> (ø)
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

lib/Db/Item.php Outdated Show resolved Hide resolved
lib/Db/Item.php Outdated Show resolved Hide resolved
@Grotax
Copy link
Member Author

Grotax commented Aug 18, 2022

While doing some research I found that phpstan can also scan for deprecation declarations.
So I added that, quite useful for nextcloud server deprecations.

lib/Db/Item.php Outdated Show resolved Hide resolved
lib/Db/Item.php Outdated Show resolved Hide resolved
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax
Copy link
Member Author

Grotax commented Aug 18, 2022

I think now I fixed every php deprecation warning I found regarding null as input for a function that expexts string doesn't make the code prettier but ok.

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax marked this pull request as ready for review August 18, 2022 14:19
@Grotax
Copy link
Member Author

Grotax commented Aug 18, 2022

I'm thinking that the json that we export should maybe only contain strings and just convert null into empty strings.

@anoymouserver
Copy link
Contributor

I'm thinking that the json that we export should maybe only contain strings and just convert null into empty strings.

In my opinion null is fine (and a valid JSON type) and the importing software could convert it itself into an empty string if necessary. Replacing all null values would also conflict with our own import (e.g. should the field be NULL or just empty in the database).
In fact, we have recently switched from an empty string to null in the API ourself.

@Grotax
Copy link
Member Author

Grotax commented Aug 18, 2022

Okay then we keep it this way :)

composer.json Outdated Show resolved Hide resolved
add phpstan deprecation rules

Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax merged commit c09cca7 into master Aug 19, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix/php8.1_depreciation branch August 19, 2022 07:14
@anoymouserver
Copy link
Contributor

You apparently overwrote some changes while force-pushing .. the latest suggested changes in OPMLExporter.php and Item.php are missing in the merged commit.

@Grotax
Copy link
Member Author

Grotax commented Aug 19, 2022

Oh unlucky I guess my rebase was not correct :(
I will try to fix it.

Grotax added a commit that referenced this pull request Aug 19, 2022
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Grotax added a commit that referenced this pull request Aug 19, 2022
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Grotax added a commit that referenced this pull request Aug 30, 2022
Changed
- Ported the admin settings to vue (#2353)

Fixed
- Fix PHP 8.1 deprecations (#1861)
- Document api item types (#1861)
- Fix deprecation warnings from Nextcloud server (#1869)
- Fix when marking all items as read, all items of the user are used in the sql query (#1873)
- Fix adding feed via the web-ui that was just deleted causing an error (#1872)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax mentioned this pull request Aug 30, 2022
Grotax added a commit that referenced this pull request Aug 30, 2022
Changed
- Ported the admin settings to vue (#2353)

Fixed
- Fix PHP 8.1 deprecations (#1861)
- Document api item types (#1861)
- Fix deprecation warnings from Nextcloud server (#1869)
- Fix when marking all items as read, all items of the user are used in the sql query (#1873)
- Fix adding feed via the web-ui that was just deleted causing an error (#1872)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
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.

None yet

4 participants