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/remove PHP 8.2 depreciation messages #397

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

krytenuk
Copy link

@krytenuk krytenuk commented Jan 2, 2024

I was getting deprecation notices after updating a project to PHP 8.2.
Have added type hints and property defaults along with a general code tidy on the worked files.

garryxigen and others added 6 commits November 1, 2023 14:58
Fixed doc-block typo and corrected some doc-block property types
Added method return types
Added int type cast
Added empty array default values
Updated doc-blocks
General code tidy
@sreichel
Copy link

Type hints are breaking changes, not?

@develart-projects
Copy link
Collaborator

As failing tests are suggesting, these are breaking changes, not really backward compatible. Therefore I'm against this PR to be merged.

@develart-projects develart-projects self-requested a review January 23, 2024 17:08
@develart-projects develart-projects added the requested change some changes are needed before the merge label Jan 23, 2024
@develart-projects develart-projects marked this pull request as draft January 26, 2024 10:25
@krytenuk
Copy link
Author

krytenuk commented Feb 4, 2024

Changed the default delimiters to null. Hopefully fixed phpunit errors.

# Conflicts:
#	library/Zend/Controller/Action.php
#	library/Zend/Session/SaveHandler/DbTable.php
@krytenuk krytenuk marked this pull request as ready for review February 4, 2024 21:33
@@ -253,7 +253,7 @@ public function setLifetime($lifetime, $overrideLifetime = null)
/**
* Retrieve session lifetime
*
* @return int
* @return bool|int

Choose a reason for hiding this comment

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

Suggested change
* @return bool|int
* @return int|false

for consistency with the property.

@develart-projects
Copy link
Collaborator

Well, we still have tons of hints, forcing some type. Like:
public function getViewScript(?string $action = null, ?bool $noController = null): string

Why exactly we are trying to be more typed here pls? Wgat are those deprecation messages about?
By merging this, consequences could be huge in terms of backward compatibility.

@IMSoP
Copy link

IMSoP commented Oct 8, 2024

My guess is this is about PHP adding "tentative return types" to built-in classes, and giving (non-error) messages like this:

Deprecated: Return type of Example::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

As the message says, the backward-compatible way of acknowledging but silencing the notice is to add a #[\ReturnTypeWillChange] attribute. If placed on its own line, this looks like a comment to older versions of PHP, so is completely safe. Note that all that does is reduce the logging of the message; the actual problem will probably have to be tackled if and when someone wants a version of this library working on PHP 9.0.

See this Stack Overflow reference question for a deeper explanation.

@develart-projects develart-projects marked this pull request as draft October 14, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requested change some changes are needed before the merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants