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

Php82 fixes #802

Merged
merged 7 commits into from
Apr 3, 2023
Merged

Php82 fixes #802

merged 7 commits into from
Apr 3, 2023

Conversation

surrim
Copy link
Contributor

@surrim surrim commented Feb 10, 2023

I fixed the most critical things in order to get S9Y working on PHP8.2.

I had to update Smarty to v4.3.0 in order to have PHP8.2 support. Without that the screen was full of warnings and critical errors. I included the package with composer, so I also had to rename some paths in the code.
I also included a strftime polyfill which seems to work great, see #784

Can we merge the improvements already made in already?
At least it is a good starting point to fix warnings because it is no longer completely broken.

@surrim surrim added the discuss label Feb 10, 2023
}
],
"require": {
"php": ">=5.3.0"
Copy link
Member

Choose a reason for hiding this comment

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

This change is strange. I see that it was not intended, but how can we solve it? voku/simple-cache has psr/simple-cache as dependency, but sets it to 1.0 || 2.0, with only 2.0 requiring PHP 8.0.

We should either bundle 1.0 (if compatible with PHP 8), somehow bundle both and use the one that fits, or swap out voku/simple-cache?

Copy link
Member

@onli onli Feb 17, 2023

Choose a reason for hiding this comment

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

I wonder whether we should replace voku/simple-cache with https://github.com/tedious/stash. Based on the composer file it only needs 7.0, though this will only be true if tedious/Stash#417 is merged (though it might only enable PHP 7.4..., unclear). Capability and usage wise it seems to be quite similar and more actively maintained. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea, I don't know any of them. I have only updated Smarty and included the strftime module - this was the result.
Fell free to modify the branch ;-)

@stevleibelt
Copy link

One general question, why merge to master and not to a feature branch like "update-to-php-8-2"?

To much complexity overhead?

Cheers and thanks for your work.

@surrim
Copy link
Contributor Author

surrim commented Feb 12, 2023

Because I've changed many files and don't want to have big merge conflicts. Nothing is rewritten, so now it's working. If somebody else is making changes, maybe the code is failing later.

@onli
Copy link
Member

onli commented Apr 3, 2023

@surrim, should we merge this and work on the remaining tasks - like the cache library - in master?

@surrim
Copy link
Contributor Author

surrim commented Apr 3, 2023

@surrim, should we merge this and work on the remaining tasks - like the cache library - in master?

Sure! If nobody works on the code, it will end up as garbage.

@onli onli merged commit e3ba81c into master Apr 3, 2023
@onli onli deleted the php82_fixes branch April 3, 2023 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants