Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up to:
#120955: Integrate Diff into Core
#1848264: Compare and merge PhpWiki diff*.php with MediaWiki's DairikiDiff.php and DiffEngine.php
Task
- Convert
Drupal\Component\Diff\*
into a proper, PSR-0-compatible PHP component.
Current work in https://github.com/DDiff/Diff
Comment | File | Size | Author |
---|---|---|---|
#25 | 1848266.25.patch | 84.28 KB | alexpott |
#22 | 1848266.22.patch | 84.3 KB | alexpott |
#22 | 17-22-interdiff.txt | 1.4 KB | alexpott |
Comments
Comment #1
andypostProbably it's time to make it active
Comment #2
andypostAlso @todo was added to
core/lib/Drupal/Core/Config/ConfigManager.php
in #2188595: Create a ConfigManager to be able to remove config.incComment #3
alexpottHere's a first cut of work - I've done this once before so I used that. We got a lot of work to tidy this up for docs, coding standards and to test.
What the patch does:
Still to do:
Comment #4
sunThanks for kick-starting this, @alexpott!
Q: Did you retain most the functional code as-is?
One of the primary goals of my original work in #120955: Integrate Diff into Core work was to retain 100% compatibility with the original Diff library of MediaWiki, so that our PSR-0-ified component can hopefully serve as a drop-in replacement for MediaWiki and any other FOSS project.
That's also the reason for the follow-up issue #1848264: Compare and merge PhpWiki diff*.php with MediaWiki's DairikiDiff.php and DiffEngine.php — ...and speaking of, before we move and convert all of the procedural code, I guess it would be best to perform that diff/review first - otherwise, it's going to be close to impossible to produce any kind of sane diff ;-)
Comment #6
alexpottDoh. Forgot to actually add the DiffEngine classes.
A: Changed as little as possible.
There is no procedural code. It is all old style PHP4 OO code :)
Changes so far:
__construct()
DrupalDiffFormatter
toDrupal\Core\Diff\DiffFormatter
- so it is allowed to have drupal specific code.getEdits()
sinceDiff::$edits
has become protectedComment #7
andypostDoes it make sense to fix this in upstream of the MediaWiki?
Quickly goggled for psr-0+mediaWiki I found that they already started conversion
Seems this broken in original code ($x0, $y0) are undefined
Comment #8
alexpott@andypost this is in a foreach loop and the $x0 and $y0 are defined later inside the loop.
I don't think we should wait for mediawiki to go psr-0 since they're not using this diff engine anymore.
Rerolled.
Comment #10
alexpott@sun has created https://github.com/DDiff/Diff to merge all the forks into one
Comment #11
YesCT CreditAttribution: YesCT commentedputting github repo in the issue summary.
Comment #12
cordoval CreditAttribution: cordoval commentedI guess i overlapped a bit https://github.com/cordoval/drupal-diff
^_^ i will continue with the tests because i think it is a different approach but i will also try to tackle other components as well if i have time
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commentedThis is critical because i am not re-opening #1929270: [meta] Drupal-agnostic components should not be calling Drupal functions
Our forked DiffEngine is dependant on whole Drupal after #2208475: Move Settings into Drupal\Core\Site\Settings
So, we either do this issue here, or we move it in Drupal\Core namespace like #2235099: Move Archiver to Drupal/Core
Comment #14
cordoval CreditAttribution: cordoval commentedis there a hope to decouple it still?
Comment #15
alexpottActually I suggest that we PSR-0 the diff formatter and decouple Drupal code from the component with the attached patch and then we can explore replacing the component with whatever sun and I come up with on https://github.com/DDiff/Diff.
Comment #16
alexpottWhilst I don't want to release D8 with the Diff component the it is I don;t think this is a critical since nothing is broken and so what it we can't make the diff a proper component.
Personally I would like to proceed with the patch here and then work on getting https://github.com/cordoval/drupal-diff or https://github.com/DDiff/Diff in.
@sun disagrees because:
As far as I;m concerned we already are in the business of maintaining a diff library and this issue does nothing to change that. It only makes swapping it out easier. I'm prioritising Drupal over a nice clean component - yes - but at the moment that nice clean well test PSR-0 diff is vapourware.
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedMaybe this supposed to be a protected method? and also update the commented line to the new method name
missses @file doc
needs newline
not sure about this
needs relocation in the config/install folder
Dunno what else is left here. Remove underscore naming? Switch remaining
var
withpublic
?Comment #18
alexpottI want to do as little as possible to the component - all this is doing is untangling the Drupal core code from the component and making the context configuration not settings which is just plain weird.
Fixed up the docs in \Drupal\Core\Diff\DiffFormatter too
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedComment #20
ParisLiakos CreditAttribution: ParisLiakos commentedsorry, wanted to trigger the bot from mobile :p
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedOk, this is good enough for me
Just a couple nitpicks for the Core class and we are good to go
This needs the full namespace
comment should be in a new line and end with a dot
Comment #22
alexpottComment #23
ParisLiakos CreditAttribution: ParisLiakos commentedthanks
Comment #25
alexpottStraight git rebase fixed the patch
Comment #26
catchCommitted/pushed to 8.x, thanks!