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

Support PHP 7.4 #37302

Merged
merged 2 commits into from
Apr 25, 2020
Merged

Support PHP 7.4 #37302

merged 2 commits into from
Apr 25, 2020

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Apr 24, 2020

Description

  • allow PHP 7.4 in console.php and index.php
  • add extra unit test pipelines in CI for PHP 7.4
  • switch acceptance test pipelines in CI from PHP 7.2 to 7.4

Note: for pipelines that use a federated server as part of the test environment, those will keep running the federated server on PHP 7.2, because the federated server will be some older release of ownCloud (e.g. 10.3.2 or 10.4.1) that we know does not support PHP 7.4.

Related Issue

#36509

How Has This Been Tested?

CI

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@phil-davis
Copy link
Contributor Author

IMO this is ready. CI is green so all the main features work with PHP 7.4. After merging this we can then think about if there are other "special" features that should be separately tested.

Please review.

if (\version_compare(PHP_VERSION, '7.4.0alpha1') !== -1) {
echo 'This version of ownCloud is not compatible with PHP 7.4' . PHP_EOL;
// Show warning if PHP 7.5 or later is used as ownCloud is not compatible with PHP 7.5
if (\version_compare(PHP_VERSION, '7.5.0alpha1') !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the next version will be php 8.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will be fine also. I put 7.5 because if there is a 7.5 then we do not know if we would automatically support it or not.

@codecov
Copy link

codecov bot commented Apr 24, 2020

Codecov Report

Merging #37302 into master will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37302   +/-   ##
=========================================
  Coverage     64.52%   64.53%           
  Complexity    19166    19166           
=========================================
  Files          1266     1266           
  Lines         74938    74952   +14     
  Branches       1331     1331           
=========================================
+ Hits          48356    48372   +16     
+ Misses        26190    26188    -2     
  Partials        392      392           
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.69% <0.00%> (+0.01%) 19166.00 <0.00> (ø)
Impacted Files Coverage Δ Complexity Δ
console.php 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
index.php 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
lib/private/Share20/DefaultShareProvider.php 97.74% <0.00%> (-0.16%) 120.00% <0.00%> (ø%)
settings/routes.php 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
lib/private/OCS/Provider.php 100.00% <0.00%> (ø) 5.00% <0.00%> (ø%)
apps/files/appinfo/routes.php 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
apps/files/lib/Capabilities.php 100.00% <0.00%> (ø) 2.00% <0.00%> (ø%)
apps/federation/appinfo/routes.php 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
apps/dav/lib/Files/Xml/FilterRequest.php 0.00% <0.00%> (ø) 13.00% <0.00%> (ø%)
apps/federatedfilesharing/appinfo/routes.php 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19f5567...b139949. Read the comment docs.

@phil-davis
Copy link
Contributor Author

Note: PHP 7.4 emits messages about array access attempts on things that are not arrays, e.g. I purposely put some rubbish code to get this message from the server output:

[Fri Apr 24 15:02:21 2020] Trying to access array offset on value of type bool at /home/phil/git/owncloud/core/apps/files_sharing/lib/SharingBlacklist.php#90

And this message in owncloud.log

{"reqId":"7jM1ctNazVLshFQSiRfq","level":3,"time":"2020-04-24T09:17:21+00:00","remoteAddr":"10.49.210.9","user":"admin","app":"PHP","method":"GET","url":"\/ocs\/v1.php\/apps\/files_sharing\/api\/v1\/sharees?format=json&search=ad&perPage=200&itemType=file","message":"Trying to access array offset on value of type bool at \/home\/phil\/git\/owncloud\/core\/apps\/files_sharing\/lib\/SharingBlacklist.php#90"}

Now that unit tests are passing, there should not be "too many" of this remaining. I will check the server and owncloud logs in CI.

@phil-davis
Copy link
Contributor Author

phil-davis commented Apr 24, 2020

https://drone.owncloud.com/owncloud/core/24446/85/17

{"reqId":"gjHXY3KH2ySQRqGuqaLe","level":3,"time":"2020-04-24T06:17:37+00:00","remoteAddr":"192.168.16.8","user":"user1","app":"PHP","method":"POST","url":"\/ocs\/v1.php\/apps\/files_sharing\/api\/v1\/shares","message":"Trying to access array offset on value of type bool at \/drone\/src\/apps\/federatedfilesharing\/lib\/FederatedShareProvider.php#255"}
{"reqId":"gjHXY3KH2ySQRqGuqaLe","level":3,"time":"2020-04-24T06:17:37+00:00","remoteAddr":"192.168.16.8","user":"user1","app":"PHP","method":"POST","url":"\/ocs\/v1.php\/apps\/files_sharing\/api\/v1\/shares","message":"Trying to access array offset on value of type null at \/drone\/src\/apps\/federatedfilesharing\/lib\/FederatedShareProvider.php#255"}
{"reqId":"gjHXY3KH2ySQRqGuqaLe","level":3,"time":"2020-04-24T06:17:37+00:00","remoteAddr":"192.168.16.8","user":"user1","app":"PHP","method":"POST","url":"\/ocs\/v1.php\/apps\/files_sharing\/api\/v1\/shares","message":"Trying to access array offset on value of type null at \/drone\/src\/apps\/federatedfilesharing\/lib\/FederatedShareProvider.php#255"}
{"reqId":"g54yEx5JKJfrRrAvDXAa","level":3,"time":"2020-04-24T06:17:58+00:00","remoteAddr":"192.168.16.8","user":"user1","app":"PHP","method":"POST","url":"\/ocs\/v1.php\/apps\/files_sharing\/api\/v1\/shares","message":"Trying to access array offset on value of type bool at \/drone\/src\/apps\/federatedfilesharing\/lib\/FederatedShareProvider.php#255"}

Needs investigating. I raised issue #37303 to follow-up these.

Edit: see the issue - 2 different "access array offset" were found.

@phil-davis
Copy link
Contributor Author

IMO this can be merged so that people can use master with PHP 7.4.

@phil-davis phil-davis requested a review from micbar April 24, 2020 12:31
Copy link
Member

@VicDeo VicDeo left a comment

Choose a reason for hiding this comment

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

it would be a bit easier for me to improve compatibility if master will have these changes

@phil-davis phil-davis merged commit edf46c2 into master Apr 25, 2020
@delete-merged-branch delete-merged-branch bot deleted the support-php-7.4 branch April 25, 2020 05:06
@phil-davis
Copy link
Contributor Author

Now that this has been merged, tonight's daily-master tarballs will allow PHP 7.4.
That will make life easier when testing apps...

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.

3 participants