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 missing return type for _getVersion() #1530

Open
wants to merge 1 commit into
base: 3.4.x
Choose a base branch
from

Conversation

Bellardia
Copy link

We're still using 3.4.x on PHP 7.4

There's a type error caused by a missing return type that was added to Phalcon\Version.

Fatal error: Declaration of Phalcon\Devtools\Version::_getVersion() must be compatible with Phalcon\Version::_getVersion(): array in /usr/share/phalcon-devtools/scripts/Phalcon/Devtools/Version.php on line 40
  • I have read and understood the [Contributing Guidelines][:contrib:]
  • I have checked that another pull request for this purpose does not exist
  • I wrote some tests for this PR

Copy link
Contributor

@rudiservo rudiservo left a comment

Choose a reason for hiding this comment

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

Phalcon 3.4 no longer maintainable.

@Bellardia
Copy link
Author

Phalcon 3.4 no longer maintainable.

It kind of is; we're running it in production on PHP 8.2. Whether or not you want these changes merged back is a choice from there. I have a much larger PR for full compatibility that there seemed to be interest for in discord, even if it only benefits a select few.

@rudiservo
Copy link
Contributor

Why have 3.4 in php8.2 and why haven't you upgraded yet to 5.

There are other issues why I can't accept this, return types where only after php7.4
Phalcon 3.4.4 is can be used in php 7, although php7 is EOL, this means not only I would break compatibility, I would have to be responsible for maintain this.

https://www.php.net/manual/en/language.types.declarations.php

I hope you understand but time is limited, I would rather focus phalcon 5 for now, if you need something to make your upgrade easier just create an issue.

@Bellardia
Copy link
Author

All good, I actually totally understand, feel free to close.
We face opposite constraints :) In this case, it's far easier to maintain 3.4.x on modern PHP than it is for me to upgrade to Phalcon 5. It'd be great to move to 5, but the changes are largely cosmetic so there hasn't been a business case.

@rudiservo
Copy link
Contributor

Current Phalcon5 is not just cosmetic, there are bug fixes, try it and let me know.
Thanks for the contribution anyway :)

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.

2 participants