websvn
websvn copied to clipboard
websvnphp
PHP 8 support ?
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.
I didn't look into PHP 8 myself, but just want to mention that WebSVN either supports Text_Diff or some DIFF tool provided by the current shell if the former is not available. Though, results are different in both cases and seemed to be correct only for Text_Diff, so we told people to use that. But if that's not compatible with PHP 8 anymore, one might need to look at why the results are wrong for the DIFF-tool on the shell and we might simply use that in future only and simply drop Text_Diff entirely.
https://github.com/websvnphp/websvn/issues/130#issuecomment-723831816
Dropping Text_Diff in favor of system diff is indeed a good workaround, and I will do that in the websvn package for Fedora 35 (which is supposed to reach beta status in the next few weeks). As a side note, CentOS Stream 9 / RHEL 9 will also ship with PHP 8, and thus will need to use system diff too.
However, if the system diff output is known to be buggy, 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.
I will try and replicate, but I would appreciate if someone more knowledgeable than me can also look into it.
However, if the system
diffoutput 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_Diffwith 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.
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 diff(1) is producing wrong ouput. I highly doubt that both GNU and BSD diff produce non-sense. It is either Text_Diff or our PHP handling is wrong.
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.
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.
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? It looks promising from the code coverage, GitHub stars and quite a number of projects are using it.
Would the package sebastianbergmann/diff be an alternative for GNU/BSD diff to be platform independent? It looks promising from the code coverage, GitHub stars and quite a number of projects are using it.
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 each(). This happens IF native mode is used. If one installs xdiff PHP extension it all works. When Text_Diff is not installed, diff(1) and the operation fails for files which contain non-ASCII characters. Will continue and try Horde_Text_Diff.
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 php-8.x. WebSVN works not with Horde_Text_Diff on PHP 8.2-beta2. I hope that @bytestream can adopt our fork and publish a new version under his packagist namespace somewhere this year.
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 Native diff impl in Text_Diff because the way Xdiff is used is that the context is always applied to the entire file and the diff is just too large.
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 horde/imap-client to run. There's 100+ horde packages in total, and just the ones I updated were tedious enough. They were originally intended as a stop gap until Horde 6 officially offers PHP 8 support. However, I'm concerned that Horde 6 may be some time and the forks will have diverged too significantly by then.
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 horde/text-diff doesn't have any dependencies that I've not released so it should be pretty simple 😅 You can take inspiration from the following commits:
- https://github.com/bytestream/Util/commit/ed3c8d589d16a0b47031087bf568272ae75677f9
- https://github.com/bytestream/Util/commit/0d8614a113417d7acea3a116fc06d3b7b5edd374
- https://github.com/bytestream/Util/commit/9a20a098a4589377afb9d975abdd9942d0649510
- https://github.com/bytestream/Util/commit/a992adb0c30875b25fe8172621d7da6c67ace05a
- https://github.com/bytestream/Util/commit/383fa9e4102f8120937104e27b9b082326e7ad6a
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? Here is my horde-text-diff branch: https://github.com/websvnphp/Text_Diff/commits/php-8.x
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 :)
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: https://github.com/maintaina-com/Text_Diff/compare/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:
# ./vendor/bin/phpunit
PHPUnit 9.5.24 #StandWithUkraine
...I....I...R... 16 / 16 (100%)
Time: 00:00.019, Memory: 6.00 MB
There was 1 risky test:
1) Horde_Text_Diff_RendererTest::testPearBug12710
This test did not perform any assertions
/var/tmp/WebSVN_Text_Diff/test/Horde/Text/Diff/RendererTest.php:269
OK, but incomplete, skipped, or risky tests!
Tests: 16, Assertions: 58, Incomplete: 2, Risky: 1.
But on your fork it does not even run:
# ./vendor/bin/phpunit
PHP Warning: require_once(Horde/Test/Bootstrap.php): Failed to open stream: No such file or directory in /var/tmp/Horde_Text_Diff/test/Horde/Text/Diff/bootstrap.php on line 7
Warning: require_once(Horde/Test/Bootstrap.php): Failed to open stream: No such file or directory in /var/tmp/Horde_Text_Diff/test/Horde/Text/Diff/bootstrap.php on line 7
PHPUnit 10.0-dev by Sebastian Bergmann and contributors.
Error in bootstrap script: Error:
Failed opening required 'Horde/Test/Bootstrap.php' (include_path='.:/usr/local/share/pear')
#0 /var/tmp/Horde_Text_Diff/vendor/phpunit/phpunit/src/TextUI/Application.php(412): include_once()
#1 /var/tmp/Horde_Text_Diff/vendor/phpunit/phpunit/src/TextUI/Application.php(233): PHPUnit\TextUI\Application->handleBootstrap('/var/tmp/Horde_...')
#2 /var/tmp/Horde_Text_Diff/vendor/phpunit/phpunit/src/TextUI/Application.php(94): PHPUnit\TextUI\Application->handleArguments(Array)
#3 /var/tmp/Horde_Text_Diff/vendor/phpunit/phpunit/src/TextUI/Application.php(77): PHPUnit\TextUI\Application->run(Array, true)
#4 /var/tmp/Horde_Text_Diff/vendor/phpunit/phpunit/phpunit(90): PHPUnit\TextUI\Application::main()
#5 /var/tmp/Horde_Text_Diff/vendor/bin/phpunit(123): include('/var/tmp/Horde_...')
#6 {main}
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.
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 lib/ version of the package. After that I'll provide you feedback and you are free to do a release on packagist. Again, thanks for the quick help.
@ralflang Thank you for helping out so quick. I have a tentative version working with FRAMEWORK_6_0 branch. Will give you an update next week.
Switched from composer to PEAR. One important problem: GeSHi 1.0.9.0 is not compatible with PHP 8 since it uses create_function() which was remove in 8.0. The PEAR channel does not contain 1.0.9.1. I have a manually patched version up and running on 8.2 and so far no issues. More on Monday.
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.