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

Added \ReturnTypeWillChange Attribute #120

Closed

Conversation

jack-worman
Copy link
Contributor

@jack-worman jack-worman commented Jan 17, 2022

- Added \ReturnTypeWillChange Attribute
- Added \ReturnTypeWillChange polyfill
- Fixed tests
@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

What uses the polyfill? Why is it needed? will 8.0 or 8.1 run without polyfill?

as this project releases all components as separate packages, keeping it in main repo has no effect on them.

@jack-worman
Copy link
Contributor Author

@glensc Php 8.0 will fail with an undefined class without the polyfill. I will work on creating a separate polyfill package to include the polyfill into all the seperate packages.

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

Symfony has polyfill package and that class, so it just need to be added as dependency of affected packages:

The exact package is https://packagist.org/packages/symfony/polyfill-php81

@jack-worman
Copy link
Contributor Author

That package requires php >=7.1, so it cannot be used

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

I'm reading what the fuzz is this attribute:

and seems it's impossible to provide the same files for PHP 9.0 and PHP < 8.1 at the same time.
or keeping the attribute would allow 9.0 also top use current files?

@jack-worman
Copy link
Contributor Author

When Php9.0 happens, the attribute will need to be removed and the actual return types will need to be typehinted.
So the required php versions would then be >=7.0

@glensc
Copy link
Contributor

glensc commented Jan 17, 2022

That package requires php >=7.1, so it cannot be used

So, I guess zf1s should start maintaining their own polyfill packages... in the technical side, I would add them under https://github.com/zf1s, each package separately without the monorepo concept. and published seldomly with own versioning.

The first one then being:

perhaps create your own version, and then transfer it to this org?

I'm thinking loud how to synchronize the changes as only @falkenhawk has owner permission

@glensc
Copy link
Contributor

glensc commented Jan 18, 2022

@falkenhawk

@jack-worman
Copy link
Contributor Author

I created the zend-polyfill package directory. So @falkenhawk just needs to make it real now

@glensc
Copy link
Contributor

glensc commented Jan 18, 2022

adding packages/zend-polyfill is against what I proposed, I proposed to create it outside this monorepo for better maintainability and to have own versioning system. technically it's not zend-framework component.

but indeed, since neither of us have have no permission to create new repos here it's best to share implementation like this now and @falkenhawk can extract to standalone repo

@glensc
Copy link
Contributor

glensc commented Jan 18, 2022

and do feature splitting like symfony does, that is not zend-polyfill but zend-polyfill-php81

they even describe that in their main package:

@partikus
Copy link
Contributor

partikus commented Jan 18, 2022

@glensc @hc-jworman the idea with custom polyfill sounds good to me. Feel free to start contributing zf1s/zend-polyfill

  • zf1s/zend-polyfill please 👍 to vote
  • zf1s/polyfill please ❤️ to vote

@glensc
Copy link
Contributor

glensc commented Jan 18, 2022

@partikus the repo doesn't exist, needs to be created first. and how do you propose to handle it, make it monorepo and do similar splitting or make split repos manually? read my comments above

@partikus
Copy link
Contributor

mono-repo approach & having everything under single repo IMHO helps to track changes & history, I would love to stick with this idea, same as symfony does. Let @falkenhawk share his experience with splitting the mono-repo and publishing packages.

@falkenhawk what do you think?

@falkenhawk
Copy link
Member

I am just a bit busy at the moment, pardon my slow response time.

@partikus let's go with zf1s/polyfill-php81 or zf1s/polyfill repo, without zend- prefix as it would not related to any historical zf1 components. Or outside of this org as @glensc suggested. Depends on what is really needed there.

btw is it the only thing needed to achieve compatibility with php 8.1, or only one of many? Just asking because I haven't looked into details yet (still these GHA issues are killing me, why do phpunit processes get killed there recently...), and I am not entirely sure we can manage the increasing maintenance 🤔

@falkenhawk
Copy link
Member

Meanwhile, I would like to thank you @hc-jworman for your huge efforts here. 💯

@jack-worman
Copy link
Contributor Author

btw is it the only thing needed to achieve compatibility with php 8.1, or only one of many?

There is most definitely more, but with this PR the deprecation error spam will stop and we can focus on other things.

Required jworman/polyfill-php81
@jack-worman jack-worman force-pushed the Add_ReturnTypeWillChange_Attribute branch from c1fe9b6 to f54a13d Compare January 19, 2022 00:35
@jack-worman
Copy link
Contributor Author

jack-worman commented Jan 19, 2022

@falkenhawk I made jack-worman/polyfill-php81#2, please fork it and create zf1s/polyfill-php81

@falkenhawk
Copy link
Member

@hc-jworman so it is also necessary to add these attributes to test files, so that when phpunit runs on php 8.1 it does not throw millions of deprecation notices? (btw I saw your PR zf1s/phpunit#5, thanks for that, I should be able to look into outstanding zf1s-related PRs soon 🤞)

@jack-worman
Copy link
Contributor Author

@falkenhawk Yes. The tests will error out sometimes because of the deprecations. With the phpunit PR, the tests almost do a complete run through, but there is some other testing dependency that needs to be updated.

@partikus
Copy link
Contributor

short update: I've forked @jack-worman 's polyfill and published it.

https://packagist.org/packages/zf1s/polyfill-php81
https://github.com/zf1s/polyfill-php81

I've decided to avoid creating not needed mono-repo for polyfills. Single repo which is needed now is much easier to maintain.

Besides that I've put some effort to zf1s/phpunit:
zf1s/phpunit#7
zf1s/phpunit#8

Copy link
Contributor

@glensc glensc left a comment

Choose a reason for hiding this comment

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

the polyfill "jworman/polyfill-php81": "^1.25" needs tbe changed to the zf1s org one

@falkenhawk
Copy link
Member

continued in #141

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.

5 participants