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

Adds PHP 8.3 as new supported version #74

Closed
wants to merge 6 commits into from
Closed

Conversation

fezfez
Copy link
Contributor

@fezfez fezfez commented Nov 3, 2023

No description provided.

Signed-off-by: Stéphane Demonchaux <[email protected]>
Signed-off-by: Stéphane Demonchaux <[email protected]>
@boesing boesing self-assigned this Nov 8, 2023
@boesing boesing added this to the 2.4.0 milestone Nov 8, 2023
@boesing
Copy link
Member

boesing commented Nov 8, 2023

Thanks, @fezfez.

Will see when I find some time to remove all changes besides composer.json.
There is no need to update psalm, especially not if you then add the issues to the baseline rather than fixing them.

So once I find some time, I will:

  • remove dependency bumps
  • remove psalm baseline change

If you find some time meanwhile, feel free to revert those changes so that we can merge this.

@fezfez
Copy link
Contributor Author

fezfez commented Nov 8, 2023

Thanks, @fezfez.

Will see when I find some time to remove all changes besides composer.json. There is no need to update psalm, especially not if you then add the issues to the baseline rather than fixing them.

So once I find some time, I will:

* remove dependency bumps

* remove psalm baseline change

If you find some time meanwhile, feel free to revert those changes so that we can merge this.

i have clean some psalm error, if you agree i prefer moving forward instead of backward, i have left two error that i think can be easily cleaned with some knowledge that i don't have

@boesing
Copy link
Member

boesing commented Nov 9, 2023

[...] i prefer moving forward instead of backward [...]

Its absolutely fine that you prefer going forward.
What I do try to avoid here is changes introduced in a PHP update which lead to BC breaks (i.e. by removing return statement and replace them with a totally different return value while ignoring the return value of the method which was invoked earlier).

Therefore, I will definitely remove these changes.
Focussing on the real topics mostly avoids accidentally introducing bugs. So unless something is necessary for PHP 8.3, please move those changes to dedicated PRs. Absolutely happy with receiving dedicated PR targeting psalm issues where we can discuss some changes, etc. - but I think that if we do not merge this, some1 will start crying once PHP 8.3 is out and badly wants this to be released.


Most of the psalm stuff cannot be changed anyways as almost any return types/property types are invalid starting in laminas-cache.

Since there will be a new major version for laminas-cache introducing 100% type-inference and which ill remove all those "pass by reference" stuff, which will then require a new major here as well, I do not see why we should spend time in fixing something which is somehow "accepted" by users.

We do have a bunch of stuff baselined regarding laminas, thats most likely since there was never a check if the types are always like they're stated (besides the fact that some1 added unit tests which only test happy-path). Therefore, we sadly have to live with the fact that psalm will report a bunch of stuff until we are able to fix these in new versions (sometimes even in majors as "making psalm happy" and "breaking stuff" is sometimes unavoidable).

@fezfez
Copy link
Contributor Author

fezfez commented Nov 10, 2023

@boesing see https://github.com/laminas/laminas-cache-storage-adapter-filesystem/pull/75/files
but without adding to baseline the build fail...

i dont understand the main reason to not run baseline, i have done it in a lot of pull request

edit : see https://github.com/laminas/laminas-cache/pull/280/files as example

@nusphere
Copy link

#80

@Xerkus Xerkus removed this from the 2.4.0 milestone Jan 8, 2024
@Xerkus Xerkus added Duplicate This issue or pull request already exists and removed Awaiting Maintainer Response labels Jan 8, 2024
@Xerkus
Copy link
Member

Xerkus commented Jan 8, 2024

Superseded by #75

@Xerkus Xerkus closed this Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate This issue or pull request already exists Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants