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

PHP 8 compatibility #149

Closed
wants to merge 8 commits into from
Closed

PHP 8 compatibility #149

wants to merge 8 commits into from

Conversation

monnerat
Copy link

@monnerat monnerat commented Jan 14, 2022

Hi Deon,
These are 4 commits targeting PHP 8 compatibility.
See the commit logs for more info.
Regards,
Patrick

This deprecated function has been removed in PHP 8.
PHP 8 deprecates the ability to have
	function whatever($arg1, $arg2='something', $arg3)

This commit reorders arguments of functions set_cached_item() and
draw_jpeg_photo() to meet this new requirement.
As PHP 8 introduces a built-in Attribute class, a name clash occurs
without this commit.

Class names are used by the Visitor class to dynamically build method
names. To avoid having to also rename the target methods, a class name
mapping is introduced for this purpose. This map may be augmented
whenever another similar case occurs.
In an '@ error suppression context, PHP 8 error_reporting() no longer
returns 0 but an error mask of errors that cannot be supressed and
passes the effective error number to the error handler (instead of 0).

Adapt the test in a compatible way.
@timiti29
Copy link

Hello,

I just install PLA 1.2.6.3 and i had the same issue, i've check on my server your pull request and it's work for me.
thank you !

Timiti29

@monnerat
Copy link
Author

... it's work for me. thank you !

You're welcome :-)

@hydrapolic
Copy link

Thanks, works with 1.2.6.3 and php 8.0.

@breisig
Copy link

breisig commented May 29, 2022

This does not work with php 8.1 [php 8.1.6]

PHP 8.1 replaces some kind of resources by built-in class instances.
As is_resource() is always used to test for failures, replace calls by
simple Boolean checks.
PHP 8.1 deprecates strftime().
PHP 8.1 deprecates float to int truncation.
@monnerat
Copy link
Author

monnerat commented Jun 1, 2022

This does not work with php 8.1

The last 4 pushed commits to this PR should improve PHP 8.1 compatibility.
However the feature deprecated by https://www.php.net/manual/en/migration81.deprecated.php#migration81.deprecated.core.null-not-nullable-internal is heavily used in PLA and there's no systematic action that can be done against that. I carefully checked occurrences of those kind of deprecated errors and fixed them but I may have missed some. Detailed comments about them are welcomed.

@breisig
Copy link

breisig commented Jun 1, 2022

@monnerat I just checked your updated branch against PHP 8.1.6 and it's working great for me doing basic tasks. I didn't test all the features but it works in general. :-)

@sylvainfaivre
Copy link

Hello @monnerat I just tried your branch, and although it works with php8, it is based on the leenooks:master branch, which does not have recent fixes.

I hit these 2 bugs with your branch :
#96
#140
They are fixed in the leenoks:BRANCH-1.2 branch, but not in master.

I'm not sure what the branching strategy is on the origin repo, but you might want to rebase your work on the 1.2 branch.

@monnerat
Copy link
Author

monnerat commented Jul 8, 2022

They are fixed in the leenoks:BRANCH-1.2 branch, but not in master.

Sure! But as the default branch for this repo is master, I based the PR on it ! I'm aware of this situation but I try to provide fixes according to the repo state. The real problem is the lack of upstream reaction.

There are also some other unfixed problems I reported several years ago... In the absence of activity here, I'm sorry but I give up trying to maintain a fixed up-to-date fork !

@mmmherma
Copy link

So sad 😢 It really worked to me.

@monnerat
Copy link
Author

So sad. It really worked to me.

You can still get the patch from this PR and apply it to the 1.2 branch: there is a single conflict, very easy to resolve.

Copy link

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Is there any repo owner reviewing this ?

@monnerat
Copy link
Author

monnerat commented Aug 4, 2022

Is there any repo owner reviewing this ?

I don't think so: last repo owner "action" on it was on Jan 6, 2022 (#147).
Probably abandoned... time for a real fork ?! (don't count on me for that).

leenooks pushed a commit that referenced this pull request Aug 5, 2022
…ected.

PHP 8.1 deprecates this feature.

Closes pull-request #149 and closes #150
@leenooks leenooks closed this Aug 5, 2022
@monnerat
Copy link
Author

monnerat commented Aug 5, 2022

Probably abandoned

Seems I spoke too fast :-) Thanks for pulling Deon.

@leenooks
Copy link
Owner

leenooks commented Aug 5, 2022

:)

So my focus on 1.2.x is rare (too many things going on), and I'm also trying to get a 2.x base out there, so more folks can start to contribute to bringing it up to speed (if they want) - thereby retiring 1.2.x. But it might be some time ... :(

@monnerat
Copy link
Author

monnerat commented Aug 5, 2022

I'm also trying to get a 2.x base out there

Do you have it published somewhere ? I can't find such a branch and master is older than 1.2.
Can I help on it ?
In the meantime and until usable in prod, 1.2 is still important !
BTW: your default branch is master. Would you prefer PRs on 1.2 ?

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.

None yet

8 participants