Follow-up to:
#120955: Integrate Diff into Core
#1848264: Compare and merge PhpWiki diff*.php with MediaWiki's DairikiDiff.php and DiffEngine.php

Task

  1. Convert Drupal\Component\Diff\* into a proper, PSR-0-compatible PHP component.

Current work in https://github.com/DDiff/Diff

Files: 
CommentFileSizeAuthor
#25 1848266.25.patch84.28 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,717 pass(es).
[ View ]
#22 1848266.22.patch84.3 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1848266.22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 17-22-interdiff.txt1.4 KBalexpott

Comments

andypost’s picture

Status:Postponed» Active

Probably it's time to make it active

andypost’s picture

Issue summary:View changes

Also @todo was added to core/lib/Drupal/Core/Config/ConfigManager.php in #2188595: Create a ConfigManager to be able to remove config.inc

alexpott’s picture

Status:Active» Needs review
StatusFileSize
new59.99 KB
FAILED: [[SimpleTest]]: [MySQL] 64,296 pass(es), 6 fail(s), and 2 exception(s).
[ View ]

Here'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:

  • In #2102499: Convert DiffEngine to settings we converted some variables into Settings. The patch attached moves these properly into Config by separating the DrupalDiffFormatter into Drupal\Core\Diff\DiffFormatter - so it is allowed to have drupal specific code.
  • Global defines to class constants
  • A couple of functions that were returning a value to class constants too
  • Adds protected and public where necessary
  • Adds a new method to Diff to getEdits()

Still to do:

  • Unit tests
  • Documentation clean up
  • Refactor protected classes stating with _ to not
sun’s picture

Assigned:sun» alexpott

Thanks 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 ;-)

Status:Needs review» Needs work

The last submitted patch, 3: 1848266.3.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new18.79 KB
new79.74 KB
PASSED: [[SimpleTest]]: [MySQL] 64,313 pass(es).
[ View ]

Doh. Forgot to actually add the DiffEngine classes.

Q: Did you retain most the functional code as-is?

A: Changed as little as possible.

before we move and convert all of the procedural code

There is no procedural code. It is all old style PHP4 OO code :)

Changes so far:

  • All classes and methods have the same name - changed methods which had the same name as their class to __construct()
  • In #2102499: Convert DiffEngine to settings we converted some variables into Settings. The patch attached moves these properly into Config by separating the DrupalDiffFormatter to Drupal\Core\Diff\DiffFormatter - so it is allowed to have drupal specific code.
  • Global defines to class constants
  • A couple of functions that were returning a value to class constants too
  • Adds protected and public where necessary
  • Started to removed _ from protected property names
  • Adds a new method to Diff to getEdits() since Diff::$edits has become protected
andypost’s picture

Does it make sense to fix this in upstream of the MediaWiki?
Quickly goggled for psr-0+mediaWiki I found that they already started conversion

+++ b/core/lib/Drupal/Component/Diff/DiffFormatter.php
@@ -0,0 +1,188 @@
+  public function format(Diff $diff) {
...
+            $this->_block($x0, $ntrail + $xi - $x0, $y0, $ntrail + $yi - $y0, $block);

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigDiffTest.php
@@ -80,10 +82,11 @@ function testDiff() {
\ No newline at end of file

+++ b/core/modules/system/config/system.diff.yml
@@ -0,0 +1,3 @@
\ No newline at end of file

Seems this broken in original code ($x0, $y0) are undefined

alexpott’s picture

StatusFileSize
new81.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,106 pass(es), 0 fail(s), and 8 exception(s).
[ View ]

@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.

Status:Needs review» Needs work

The last submitted patch, 8: 1848266.8.patch, failed testing.

alexpott’s picture

@sun has created https://github.com/DDiff/Diff to merge all the forks into one

YesCT’s picture

Issue summary:View changes

putting github repo in the issue summary.

cordoval’s picture

I 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

ParisLiakos’s picture

Priority:Normal» Critical

This 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

cordoval’s picture

is there a hope to decouple it still?

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new83.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,677 pass(es).
[ View ]
new3.12 KB

Actually 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.

alexpott’s picture

Priority:Critical» Major

Whilst 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 we discussed already, the code not only needs psr-ification, but also some good amount of clean-up, which will probably result in API changes... thus, swapping it out after beta is rather unrealistic and Drupal core will effectively be in the business of maintaining a diff library.

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.

ParisLiakos’s picture

Status:Needs review» Needs work
  1. +++ b/core/lib/Drupal/Component/Diff/Diff.php
    @@ -0,0 +1,172 @@
    +    //$this->_check($from_lines, $to_lines);
    ...
    +  public function check($from_lines, $to_lines) {

    Maybe this supposed to be a protected method? and also update the commented line to the new method name

  2. +++ b/core/lib/Drupal/Core/Diff/DiffFormatter.php
    @@ -0,0 +1,144 @@
    +<?php
    +
    +namespace Drupal\Core\Diff;

    missses @file doc

  3. +++ b/core/modules/config/src/Controller/ConfigController.php
    @@ -50,6 +51,12 @@ class ConfigController implements ContainerInjectionInterface {
    +  protected $diffFormatter;
    +  /**

    needs newline

  4. +++ b/core/modules/system/config/schema/system.schema.yml
    @@ -147,6 +147,38 @@ system.date_format.*:
    +system.fast_404:
    +  type: mapping
    +  label: 'Fast 404 settings'
    +  mapping:
    +    enabled:
    +      type: boolean
    +      label: 'Enabled'
    +    paths:
    +      type: string
    +      label: 'Paths'
    +    exclude_paths:
    +      type: string
    +      label: 'Exclude paths'
    +    html:
    +      type: string
    +      label: 'HTML'

    not sure about this

  5. +++ b/core/modules/system/config/schema/system.schema.yml
    --- /dev/null
    +++ b/core/modules/system/config/system.diff.yml

    +++ b/core/modules/system/config/system.diff.yml
    +++ b/core/modules/system/config/system.diff.yml
    @@ -0,0 +1,3 @@

    @@ -0,0 +1,3 @@
    +context:
    +  lines_leading: 2
    +  lines_trailing: 2

    needs relocation in the config/install folder

Dunno what else is left here. Remove underscore naming? Switch remaining var with public ?

alexpott’s picture

StatusFileSize
new5.24 KB
new84.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,662 pass(es).
[ View ]
  1. Not going to touch this until this should be done in the whatever we do to make the component
  2. Fixed
  3. Fixed
  4. Yep removed... fixed
  5. Fixed

I 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

ParisLiakos’s picture

Status:Needs work» Reviewed & tested by the community
ParisLiakos’s picture

Status:Reviewed & tested by the community» Needs review

sorry, wanted to trigger the bot from mobile :p

ParisLiakos’s picture

I 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.

Ok, this is good enough for me

Just a couple nitpicks for the Core class and we are good to go

  1. +++ b/core/lib/Drupal/Core/Diff/DiffFormatter.php
    @@ -0,0 +1,216 @@
    +   * @param ConfigFactoryInterface $config_factory

    This needs the full namespace

  2. +++ b/core/lib/Drupal/Core/Diff/DiffFormatter.php
    @@ -0,0 +1,216 @@
    +    foreach ($add as $line) {  // If any leftovers

    comment should be in a new line and end with a dot

alexpott’s picture

StatusFileSize
new1.4 KB
new84.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1848266.22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

thanks

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 22: 1848266.22.patch, failed testing.

alexpott’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new84.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,717 pass(es).
[ View ]

Straight git rebase fixed the patch

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.