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

feat: Support for PHP-8 #181

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

erkenes
Copy link
Contributor

@erkenes erkenes commented Mar 4, 2022

Adjust the classes and dependencies to support PHP-8.1

Resolves: #170
Resolves: #175

Adjust the classes and dependencies to support PHP-8.1

Resolves: heiseonline#170
Resolves: heiseonline#175
@richard67
Copy link
Contributor

I have merged this PR into a branch on my fork and have tested it and can confirm that it works.

So this PR is ok, maybe except of some code style changes with which I'm not happy, so I've reverted them in my fork.

@liayn
Copy link
Contributor

liayn commented Oct 20, 2023

@richard67 Thanks for the info and your work.

Code-Style-wise I highly recommend sticking to PSR-12. https://www.php-fig.org/psr/psr-12/

Please let me know, when you merged your branch into your master branch.
I will then finally take your sources for Shariff and this backend for the official TYPO3 integration.
Many people will be happy about this major move forward!

@richard67
Copy link
Contributor

Code-Style-wise I highly recommend sticking to PSR-12. https://www.php-fig.org/psr/psr-12/

@liayn I know it, we use it for Joomla, of which I'm a maintainer.

I will then finally take your sources for Shariff

You mean also https://github.com/richard67/shariff-plus for the frontend? Or only my fork of the backend?

My shariff-plus has - in addition to shariff - a "Like" button for Facebook, but I would not recommend to use that button in EU because it doesn't work since Facebook have hardened their data protection. Besides that button, the difference to shariff is that shariff-plus it up to date with the latest changes merged in or proposed for shariff. I have created a new release 2.3.0 yesterday of that.

My fork of the backend I will maybe make ready tomorrow or Sunday and do a release (or pre-release or draft release, as long as I'm not sure if this repo here is really abandoned or not).

If you after that decide it to use for the official TYPO3 integration, give me a note so I can properly mention it in my README.md.

But to be honest, I'm not really sure if I want to take that responsibility. I'm doing this only as hobby, and I cannot promise to maintain all forever. But at least as long as I use it for my website I will keep it alive.

@liayn
Copy link
Contributor

liayn commented Oct 20, 2023

Yes, we will use frontend and backend.

My shariff-plus has - in addition to shariff - a "Like" button for Facebook, but I would not recommend to use that button in EU because it doesn't work since Facebook have hardened their data protection....

That's fully up to the integrator anyways, which button they wanna use.

If you after that decide it to use for the official TYPO3 integration, give me a note so I can properly mention it in my README.md.

Our TYPO3 extension: https://extensions.typo3.org/extension/rx_shariff

Sure, I'll ping you.

But to be honest, I'm not really sure if I want to take that responsibility. I'm doing this only as hobby, and I cannot promise to maintain all forever. But at least as long as I use it for my website I will keep it alive.

Don't worry about this. We lived with the non-maintained original way too long actually. Any improvement is an improvement.
(And we maintain our extension also for free.)

@richard67
Copy link
Contributor

@liayn I would like to clarify a few questions, e.g. which minimum PHP version we should require, or in other words if we still should support 7.4 and 8.0, but I think we should not pollute this PR here with unrelated discussions. Maybe we continue with email in German language? You could send an email to admin<at>richard-fath<dot>de so I could reply.

@richard67
Copy link
Contributor

richard67 commented Oct 21, 2023

@liayn I've meanwhile released a new version 2.3.1 of my shariff-plus frontend, see https://github.com/richard67/shariff-plus/releases . Check also the change log for version 2.3.0 from 2 days ago to see what has changed, compared to heiseonline.

In my fork of the this repository here I've changed the default branch to 10.0-dev and just created a pre-release 10.0.0-alpha1. See https://github.com/richard67/shariff-backend-php/releases . It contains the changes from this PR here and other open PRs in this repository here.

Attention: That pre-release increases the PHP minimum version to 8.1, i.e. drops support for PHP 7.x and 8.0. All dependencies are updated to their latest version.

More attention: I am using the new releases on my website (URL is in my GitHub profile), and it seems to work. But I have tested only with PHP 8.1, not with 8.2 or 8.3, and I have tested the backend only with standard cache and client configurations.

The master branch I've kept in case if it needs to create also a version 9.1.0 which still supports PHP 7.4 and 8.0, but I'd prefer to avoid that.

Let me know your opinion by email to admin<at>richard-fath<dot>de.

Thanks in advance.

Update 2023-10-21 18:48 (UTC): I've just updated the zip and tar.gz packages of the 10.0.0-alpha1 release of my backend fork. The "shariff-backend-php-10.0.0-alpha1-dev" packages were wrong, the right ones are "shariff-backend-php-10.0.0-alpha1" (without "-dev" at the end of the base name).

@digitalgopnik
Copy link
Contributor

Hello @erkenes!
Thanks for your work on support php8 for our shariff-backend.
We will merge this pull request, even though there are some changes which affect codestyle and are not PSR-12-confirm.

We see definitely the necessity of integrating phpcs-fixer or codesniffer! But as soon there aren't any checks or guidelines from our side, we won't stop the show. 🙂

There will be a pull-request soon, which will integrate them.

@liayn @richard67 Thanks also for your work and interest in our shariff-backend

@digitalgopnik digitalgopnik merged commit e003dfd into heiseonline:master Oct 25, 2023
@richard67
Copy link
Contributor

@digitalgopnik Unfortunately this PR resulted in the composer.json and composer.lock not being consistent. When running "composer install" on a clean master branch, you get:

Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. It is recommended that you run composer update or composer update <package name>.

When running "composer update" or when removing the composer.lock file, I get:

Your requirements could not be resolved to an installable set of packages.

Problem 1

  • guzzlehttp/guzzle[7.4.1, ..., 7.8.0] require php ^7.2.5 || ^8.0 -> your php version (7.2.0; overridden via config.platform, actual: 8.1.2) does not satisfy that requirement.
  • Root composer.json requires guzzlehttp/guzzle ^7.4.1 -> satisfiable by guzzlehttp/guzzle[7.4.1, ..., 7.8.0].

I think all this should not happen on a clean branch.

Shall I open a new issue for that?

I have only a dev environment with PHP 8.1 here, so I can't really fix that for older versions.

Or do you plan anyway to enlarge the version requirements?

@digitalgopnik
Copy link
Contributor

digitalgopnik commented Oct 25, 2023

@richard67 Thanks for your report! 🙏 Somehow i disregarded that. Will take care of it, there's not needed another issue 🙂

@digitalgopnik
Copy link
Contributor

@richard67 Should be fixed now. Thanks again for your quick report!

@richard67
Copy link
Contributor

@richard67 Should be fixed now. Thanks again for your quick report!

@digitalgopnik Thanks. Last issue: The new release 10.0.0 is missing the 2 assets "shariff-backend-php.tar.gz" and "shariff-backend-php.zip".

@richard67
Copy link
Contributor

P.S.: The CHANGELOG.md should be updated for 10.0.0, too.

@digitalgopnik
Copy link
Contributor

@richard67 Thanks for your attention! Should be correct now 🙂

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.

guzzlehttp/guzzle 7+ support? Support for PHP 8
4 participants