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
PHP 8 support ? #153
Comments
I didn't look into PHP 8 myself, but just want to mention that WebSVN either supports |
Fully agree with @ams-tschoening |
Dropping |
However, if the system `diff` output is known to be buggy,
I expect the problem being in WebSVN and/or Text_Diff instead. Just
think about how man people NOT use either of both, but some system
DIFF provided by GIT, SVN or alike each day.
What would be interesting is if different of those DIFF behave
differently and if so, which one we focus on. In my linked comment the
DIFF of my GIT was used for PATH-reasons, but as WebSVN focuses on
SVN, it might make sense to explixitly require that instead and only
get that working at all.
and as
we won't be able to rely on `Text_Diff` with PHP 8, maybe #130 needs
to be re-opened ? I'm not sure that's what @ams-tschoening implies in his answer.
Either that or we keep discussing things here for now, as the other issues have already a lot of comments. Isn't that important in the end.
|
FTR: horde/Text_Diff#3 |
Looking at https://www.php.net/supported-versions.php I guess that 7.4 will truly go away by end of the year. We should try clarify whether either Text_Diff or |
If Text_Diff is deprecated for system diff what is the correct path to do that on Windows? |
There is no system diff on Windows. You need to install a third party impl. |
Sorry, I wasn't clear with my previous question. I know there is no diff on Windows so what third party diff tools are supported for that? I didn't see any references in the documentation on what could be used. |
GNU diff is available for Windows. Don't know about BSD diff. |
Would the package sebastianbergmann/diff be an alternative for GNU/BSD diff to be platform independent? |
That would require someone to write code and for downstream packages it would require to bother with composer to make it available. Not saying it is impossible, but effort. What are the huge differences between GNU and BSD diff for our usecase here? |
Update: I have set up a clean FreeBSD jail with PHP 8.2 beta and will drill through next weeks. What will not work anymore, at least with 8.2 is PEAR. I will not try yo fix it, but move to composer 2.4.1 and drop things like text_diff. When this is done, I will work on a 3.0.0 branch for PHP 8.x. After that I will try 8.1 as well and make it work with both, but not PEAR anymore. I know that composer is problematic for downstream maintainers because of packaging, but let's see what is possible. I do not intend to test with 8.0. Don't expect a release before November. I will fix issues to make it run smoothly, but I will not rewrite parts/components, e.g., diff module, in this release. |
Here is the branch: https://github.com/websvnphp/websvn/tree/php-8.x Text_Diff was installed by composer, but I wasn't able to get any erratic behavior..will continue on Monday. |
I was able to reproduce the failure with |
Made progress: There is no usable Horde_Text_Diff on packagist. I had to fork it because it not PHP 8 compatible. @bytestream has forked a lot of the Horde modules, but not Text_Diff. Looking at the logical changes applied to those I have applied similar to Text_Diff in branch Some of the changes aren't PHP 8 related, but I have noticed while testing, I will open up PRs for them. Continuing tomorrow... |
I have intentionally switched to the |
One note on PEAR: I wasn't able to make it run with PHP 8.2, it just crashes and Horde_Text_Diff isn't available in a newer version anyway, I am considering to drop PEAR completely because it does not work. I'd go for composer 2 altogether. |
@michael-o my forks cover all of the packages necessary to get I would recommend the same as what I told the Nextcloud devs; if you need a package which I haven't released to packagist then you should fork it yourself and release it (for example https://packagist.org/packages/nextcloud/horde-smtp). You're in luck that
Let me know if you need any help. |
@bytestream I did indeed check Util yesterday already and happily used your approach. Kudos for that. All tests pass with PHP 8.2-beta2. From your point of view it does not make sense to have bytestream/horde-text-diff since you don't need it for the Web Mail Client, but it should rather live as websvnphp/horde-text-diff? PS: thanks for giving extensive feedback. Appreciated! |
Let's just incorporate the patches and release horde/text_diff on packagist rather than make things too complicated :) |
@ralflang Looking into your fork... |
As I said, I will need a day or two to.polish unit tests and phpstan level, conversion to PSR-4 and PER-1. I have added the as-is version to packagist nevertheless. |
@ralflang Here is the diff: maintaina-com/Text_Diff@FRAMEWORK_6_0...websvnphp:Text_Diff:php-8.x I would start to create PRs against your fork, if you agree. |
Tests also look good for me on my fork:
But on your fork it does not even run:
|
Give me a day or two to care about the basics. Your PR's are much appreciated. There is some danger of duplication with boilerplate measures like upgrading the phpunit xml, the .horde.yml file (which autogenerates the composer.json file), gitignore file, namespacing efforts etc but it's not much trouble I guess. |
Good good! In my fork I have removed duplicate management files as done by @bytestream. No reason to duplicate GitHub milestones or a single changes file. Too many files having the same content again. |
@ralflang I have now created all of the PRs to align the code. As soon as those are approved, I will port from our fork to your fork by using the |
@ralflang Thank you for helping out so quick. I have a tentative version working with Switched from composer to PEAR. One important problem: GeSHi 1.0.9.0 is not compatible with PHP 8 since it uses |
Yes, I have a similar issue with another post pear library. Asked that maintainer to release an already existing patch from master to a new tag but no action. FriendsOfPHP5 has a polyfill for create_function to buy some time. |
Both #171 and #175 have been tackled. I have upload a possibly final branch with all open PRs combined: https://github.com/websvnphp/websvn/tree/version-next Will go over to testing with PHP 7.4 again and then 8.1. |
Swapping the PHP version on FreeBSD ist much easier than I thought, takes less than 10 min. This is now my test matrix with branch I am basically waiting for the PRs to be approved and the Horde components to be released. Then I can release WebSVN 2.8.0. So basically eight configurations work for me. If any of the participants here wants to test master before I push out the release next week or so, let me know, please. |
@k10blogger @ams-tschoening Guys, if you have some spare time, please have a look at the open PRs until 2022-10-06. I will merge then and produce 2.8.0. |
I won't have any spare time to test things and am in the process of abandoning WebSVN altogether in favour of GitLab. So don't count on me anymore and feel free to proceed as however you wish. |
Sad to hear this, though I use both and many of our users are mentally not capable to grasp Git, but I will wait for @k10blogger to review. |
@michael-o I pull the PR submitted into my instances, seems to work. Though didnt extensively check. |
Version delivered: https://github.com/websvnphp/websvn/releases/tag/2.8.0 |
FTR: The FreeBSD port has been updated on head and quarterly to 2.8.0. It uses Composer 2 for the moment. Other OS integrators are highly welcome. Enjoy! |
websvn mandates Text_Diff pear module. However Text_Diff has been unmaintained for a long time and maintenance taken over as Horde_Text_Diff. Text_Diff makes use of each() function which has been deprecated in PHP 7.2 and removed in PHP 8. Horde_Text_Diff seems to be broken too with PHP 8, likely for the same reason, which probably voids requesting a change from Text_Diff to Horde_Text_Diff in websvn.
Has anyone tried to run websvn with PHP 8 yet ?
See https://bugzilla.redhat.com/show_bug.cgi?id=1987850#c7 for more context.
The text was updated successfully, but these errors were encountered: