Convert Diff into a proper, PSR-0-compatible PHP component

Probably it's time to make it active

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

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

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

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

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

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

putting github repo in the issue summary.

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

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

is there a hope to decouple it still?

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.

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.

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 ?

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

sorry, wanted to trigger the bot from mobile :p

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

thanks

Straight git rebase fixed the patch

Committed/pushed to 8.x, thanks!