Add a "diff" of some kind to the CMI UI

Would it be reasonable to call a system diff function?

This GitElephant "abstraction layer to manage your git repositories with php" provides a DiffCommand.php wrapper. It may also provide other desirable functionality too.

I found this while searching for a #1825344: 3-way Diff View and started to think that diff engines in php may not be adequate for the configuration system's needs.

tagging

I really don't want to be integrating with a system command, we have to support far too many diverse system setups and platforms to be able to rely on that. I really think our only option is a PHP-space library (and remember that for anything existing it has to be GPL2+).

I agree that a native PHP library is needed. Please evaluate: #1826156: Move to Text_Diff library.

I started looking into various native PHP diff libraries today. There are two things I think we need for any library

1) It has to be compatible with GPL2+ (so it has to be a license in this list: http://www.gnu.org/licenses/license-list.html#GPLCompatibleLicenses). This is a hard requirement. In the past we have approached authors about dual or re-licensing their libraries and had success.
2) It should be fully OO and compatible with PSR-0 autoloading. This is not a hard requirement, however it will be tough to sell any library to the community that doesn't support PSR-0. Another possibility would be to rearchitect such a library and push the changes upstream if its not too much work.

So with these in mind, here are the libraries I've found.

• Horde_Text: License is LGPL 2.1 which is GPL compatible, however the classes are not compatible with PSR-0 autoloader.
• phpspec/php-diff (https://github.com/phpspec/php-diff): This is using the Modified BSD license which is compatible. It is also compatible with PSR-0 and composer. YAY!
• tyler-king/php-diff (https://github.com/tyler-king/php-diff): Freely licensed with no restrictions, which is not compatible with the GPL.
• phpwiki diff class (http://drupalcode.org/project/diff.git/blob/refs/heads/7.x-3.x:/DiffEngi...): GPL but not structured for PSR-0.

Given this, I'm going to head into an analysis of the phpspec library, and see where we're at since that is path of least resistance.

I got the example for the phpspec library running. I haven't evaluated the code yet, but here's how it looks by default with an image style that has an update and a new effect added.

The default output comes with various css classes that can be used to adjust its appearance.

For reference here is the config import screen

Webchick and I looked for at this for a bit and we came up with this proposal for implementation.

• We don't need diffs for new or removed config.
• For each changed file, we would add an arrow icon which would expand the diff underneath when clicked.
• There would also be an 'Expand All'/'Collapse All' button at the top.

Webchick originally suggested that they all be expanded by default, but after some discussion we thought that would be confusing to non-technical users. Much of the data in the files isn't really understandable without any context, and diffs are kind of confusing if you've never seen one before.

This would be a basic implementation we can get done and see how it works.

I had a chat with @heyrocker, I would advise against placing the diff inside the table. Having done a few in-table patterns, I learned its extremely hard to accomplish both from a UI perspective and A11y perspective.

I imagine doing an operation column with diff option, that would allow you to see the diff in a model. In that way you preserve the larger context of the thing you are diffing (something that gets lost if you do it inside a table) and you are able to do more fancy styling.

Another advantage of the approach Bojhan recommends is that we could lazy-load the diffs, whereas with a collapse/uncollapse display we have to do all the diffs on every page load whether we ever show them or not.

Here's the start of a patch for this. It includes the PHPspec Diff library and a utility function for diffing changes between two config storages, just to get things started. It doesn't expose anything on the front end at all. Despite spending over an hour discussing it with RobLoach and Crell, I was unable to get the autoloader working properly, something to do with our versions of the vendor libraries being outdated. So I just hacked it in and we can fix it in followups.

Bojhan mentioned that he would like to use modals for this, however I see that there is currently no standardized way for implementing modals in core at this point (discussions happening at #1175830: Update to jQuery UI 1.10.2 and #1667742: Add abstracted dialog to core (resolves accessibility bug)). So I'm not quite sure how/if we should go forward with that implementation, it may be better to do the expanded divs.

Here's a new patch.

- Now shows a diff inline on the config admin screen for every change. I think this is overkill but it demonstrates how things work.
- Removed the php-diff examples directory, since I assume we would do that anyways and it makes the patch smaller.
- Added a config.admin.css for styling of the diff. It is just a set of defaults from the original example.

Here's how it looks. I don't think it makes any sense to show diffs for new/deleted but maybe others disagree.

I need some help doing the show/hide stuff if anyone wants to get involved on that end. We'd need to add an operations column with 'Show Diff', and have it expand underneath. The styling could use some help too.

Related: #120955: Integrate Diff into Core

For consistency i think it makes sense to show created/deleting config as well.

Let's hide the difference by default.

Couldn't we just create the renderer from outsite optionally.

I'd been playing with the latest dev version of the DIff module and migrating it to D8 (http://drupal.org/sandbox/alexpott/1788442)

So here's a patch that includes the Diff module's engine as a Drupal Component all PSR-0'd :) and uses it for config diffs.

Here a very subjective comparison of phpspec/php-diff vs the Diff module's engine

Tests: It's a tie - neither have any!
Prettyness out of the box: phpspec/php-diff
Output overrideable by Drupal theme layer: Diff module
Drupal community knowledge: Diff module
Modernity: Hmmm... most of phpspec/php-diff's code is two years old but apparently Diff module's library has not changed much since 4.7!

I also think we should recognise the work the Diff module maintainers have done upgrading the module for the drupal.org 7 release.

This patch should not be consider complete... the Diff component needs work conform to coding standards and we need to have some text about where it came from.

Transcript of an IRC conversation re diff libraries.

dawehner__: do we really want so much custom diff code in core?
dawehner__: i don't care that much, but putting this into core adds the pressure to actually maintain it
alexpott: true
alexpott: I don't think it would be a difficult choice if there was a well maintain php diff library available..
alexpott: but the fact that phpspec/phpdiff has no tests concerns me.
alexpott: and there is a nice english phrase "Better the devil you know"
alexpott: plus just because something is in vendor doesn't mean it's not going to be a problem for us
dawehner__: hehe good point, i agree now
alexpott: I'm not overly fussed either as I'm not Mr. Diff… I'm trying to reach out to Alan D (current maintainer of Diff who has done loads of work for the latest release).
dawehner__: it's good to know that diff module has a quite abstract reusable class, so i guess someone could take the effort and write tests
alexpott: and maybe (fingers crossed) that can be a post freeze task

I was the maintainer of diff module way back when. I can say that I *never* had to change the diff class. It is the sort of code that stabilizes quickly and then you really never change it. The requirements are quite narrowly scoped.

My biggest concern with the Diff module's code is that we are essentially forking an existing open source library. I talked with Moshe about this some yesterday and he pointed out that this code hasn't changed in years, and its not like we have to worry about merging in upstream fixes or anything. This is certainly true, its stable solid code and I do like the idea of bring in existing community projects if they're worthwhile and having it be themable out of the box is a plus. I'll take a look at this patch in the coming days.

Spent some time looking at this today. Ultimately I think I'm moving in favor of this approach. We are taking on code that needs to be maintained as opposed to a vendor library, but as moshe pointed out, its not like this code changes much. It's pretty stable. I'd be interested in hearing from the current maintainers of Diff and getting their input / help on this patch.

Some other thoughts:

- Docs and coding standards need a lot of work. Although the good thing is that we could actually do them (as opposed to a true vendor library which we wouldn't touch.)
- It seems like the formatter classes could use some architecture help. (creating and implementing a proper interface, potentially making them all descend from a common abstract base class.)
- I'm not sure how much sense it makes to include classes like MappedDiff and WordLevelDiff in core if we aren't using them. While this code may not change much, we should also be mindful not to take on maintenance of code we aren't using. They could easily be supplied by contrib.

And some nitpicks

+++ b/core/includes/config.incundefined
@@ -295,3 +298,42 @@ function config_get_module_config_entities($module) {
+ * @param array $options
+ *   An array of options for formatting the array. Available options are
+ *     'ignoreWhitespace' - Boolean, defaults to FALSE.
+ *     'ignoreCase' - Boolean, defaults to FALSE.
+ *     'ignoreNewLines' - Boolean, defaults to FALSE.
+ *     'context' - Integer representing how many lines of context to show
+ *                 around the individual differences. Defaults to 3.

This should be removed since the options parameter is gone now.

+++ b/core/modules/config/config.admin.incundefined
@@ -40,6 +40,12 @@ function config_admin_sync_form(array &$form, array &$form_state, StorageInterfa
+    // If this is a change, add the CSS for the inline diff.
+    if ($config_change_type == 'change') {
+      $form['#attached']['css'] = array(drupal_get_path('module', 'system') . '/system.diff.css');
+    }

Since we're showing diffs for deleted and inserted now, this css should just be included all the time now.

+++ b/core/includes/config.incundefined
@@ -295,3 +298,42 @@ function config_get_module_config_entities($module) {
+  return theme('table', array('rows' => $formatter->format($diff)));

I liked the 'New' and 'Old' headers we had before and they would be easy to add back in.

The entire discussion about introducing a Diff library really belongs into the generic #120955: Integrate Diff into Core issue, not here. :-/

Diff module's DiffEngine class was apparently refactored in major ways over the years, so I don't really buy @moshe's comment ;)

But again, the issue of adding a diff view functionality to Config module's UI is entirely different to picking a/the proper diff library for Drupal core. Let's postpone this issue on that decision, please. (Not long ago, @heyrocker expressively hated the fact that we're performing significant architectural changes in seemingly unrelated/minor issues, and this issue is really just an extrapolation of that bad habit.)

Well, while this issue is not unrelated or minor, I can also admit I got a little caught up in wanting to get this in before freeze. It would be easy to break out the actual diff library into a new patch. I'll write a summary of the findings from above and move discussion into that issue along with a patch for the existing library. Hopefully we can keep things rolling over there.

#120955: Integrate Diff into Core is in, so I think this can proceed by taking out the engine decision part, and building on the diff engine that is now part of core to do the ui.

I don't think the proposed UI makes much sense, the diff should be in a modal..

@webchick I thought these kind of UI things, we can put in after feature freeze. I don't care if for some deadline reason we put something ugly in, but it seems like a silly strategy if we can just fix it properly once we have a modal.

New patch now the engine is in, with the nitpicks fixed from #19

Failing tests is going to be fixed by #1847828: [HEAD BROKEN] DrupalUnitTestBase and this container and transliteration and kernel and whatnot

#26: 1821548-26.patch queued for re-testing.

+++ b/core/includes/config.inc
@@ -295,3 +296,46 @@ function config_get_module_config_entities($module) {
+  // Implementers of StorageInterface::read() return keyed arrays of
+  // configuration data. The diff class wants an array of strings, each
+  // representing one line of the file we're diffing. So we convert the config
+  // array to YAML, then explode it into an array that the differ can deal with.
+  $dumper = new Dumper();
+  $dumper->setIndentation(2);
+
+  $source_data = explode("\n", $dumper->dump($source_storage->read($name), PHP_INT_MAX));
+  $target_data = explode("\n", $dumper->dump($target_storage->read($name), PHP_INT_MAX));

I think this comment could be clarified into something along the lines of:

"The output should show configuration object differences formatted as YAML. But the configuration is not necessarily stored in files. Therefore, they need to be read and parsed, and lastly, dumped into YAML strings."

+++ b/core/modules/config/config.admin.inc
@@ -40,6 +40,10 @@ function config_admin_sync_form(array &$form, array &$form_state, StorageInterfa
+    $form['#attached']['css'] = array(drupal_get_path('module', 'system') . '/system.diff.css');

We can use [] instead of array() here.

+++ b/core/modules/config/config.admin.inc
@@ -63,8 +67,35 @@ function config_admin_sync_form(array &$form, array &$form_state, StorageInterfa
+          break;
+        case 'create':
...
+          break;
+        case 'delete':

Let's add a blank line between the switch cases to clarify that these are not fall-through situations.

+++ b/core/modules/config/config.admin.inc
--- /dev/null
+++ b/core/modules/system/system.diff.css

The CSS needs to be cleaned up and brought up to par with current core standards, but I think that can be done in a follow-up issue (which should be created ahead of time though).

Something like this ? I'd would also argue that we can write tests when we're actually refactoring the Diffengine class, no ?

#25: My concern is less about feature freeze and more about sitting around and waiting for a feature that may or may not get in at all. This way we know we will have something and can start using it, rather than sitting around waiting for a patch to land who knows when. I'm happy to look at followups to improve the UI as we move along, and my understanding is the same as yours - it can be done through code freeze.

The last patch was broken because DrupalDiffFormatter calls several theme functions which no longer exist. I just removed these calls to get actual data displaying again. That's the only change in this patch. Long term I assume this will be dealt with in #1848266: Convert Diff into a proper, PSR-0-compatible PHP component.

Could really use a front-ender to make it look a little nicer.

We have modals.

#33: 1821548-import-diff-33.patch queued for re-testing.

Here's a go at switching to dialogs api. A couple of issues still need to be resolved:

1. The initial dimensions of the dialogue seem far too small.
   I'm guessing this is because the table doesn't have a set width. We probably want the dialogue to automatically take up ~70% width of larger displays but I'm not sure if this is a problem that should be sorted here as it will probably affect a number of other pages.
2. The back link causes the page to reload.
   Again this seems like it's going to be a common problem and so should probably be dealt with in another issue.

A couple of screenshots.

Hmmm, I;ve tried this patch on Chrome, Firefox, and Safari (all on OSX) and when I click the Diff link I get the throbber and 'Please wait...' and then it goes away and ... no dialog. Nothing happens at all in fact. This happens both with the Overlay on or off. Any chance you missed getting a piece of this into the patch?

Oops looks like I missed the css file - although I'm not sure that would explain what you were seeing. My patch does add in a new menu route so you'll need to rebuild the router cache for it to work.

I've fixed the second issue. The trick is to add a 'dialog-cancel' class to the close button. As for the second issue, I've posted a question in this issue: https://drupal.org/node/1667742#comment-6846256

#40: 1821548-import-diff-40.patch queued for re-testing.

Interesting. I don't think this failure is related to this patch. This is the trace:

exception 'PDOException' with message 'SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php:58
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php(58): PDOStatement->execute(Array)
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php(523): Drupal\Core\Database\Statement->execute(Array, Array)
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Driver/mysql/Insert.php(34): Drupal\Core\Database\Connection->query('INSERT INTO {ke...', Array, Array)
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Query/Merge.php(424): Drupal\Core\Database\Driver\mysql\Insert->execute()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.php(95): Drupal\Core\Database\Query\Merge->execute()
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/system.module(3022): Drupal\Core\KeyValueStore\DatabaseStorage->set('system.theme.da...', Array)
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(211): system_rebuild_theme_data()
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(105): system_list('module_enabled')
#8 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(919): module_list()
#9 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(830): module_hook_info()
#10 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(1000): module_implements('stream_wrappers')
#11 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/file.inc(195): module_invoke_all('stream_wrappers')
#12 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/file.inc(251): file_get_stream_wrappers()
#13 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/file.inc(288): file_stream_wrapper_get_class(false)
#14 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/file.inc(503): file_stream_wrapper_valid_scheme(false)
#15 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(853): file_prepare_directory('sites/default/f...', 3)
#16 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php(649): Drupal\simpletest\TestBase->prepareEnvironment()
#17 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/field/lib/Drupal/field/Tests/FieldTestBase.php(22): Drupal\simpletest\WebTestBase->setUp()
#18 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(696): Drupal\field\Tests\FieldTestBase->setUp()
#19 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(381): Drupal\simpletest\TestBase->run()
#20 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('183', 'Drupal\field\Te...')
#21 {main}

Next exception 'Drupal\Core\Database\DatabaseExceptionWrapper' with message 'SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction: INSERT INTO {key_value} (name, collection, value) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array
(
    [:db_insert_placeholder_0] => system.theme.data
    [:db_insert_placeholder_1] => state
    [:db_insert_placeholder_2] => a:3:{s:6:"bartik";O:8:"stdClass":8:{s:3:"uri";s:30:"core/themes/bartik/bartik.info";s:8:"filename";s:30:"core/themes/bartik/bartik.info";s:4:"name";s:6:"bartik";s:4:"info";a:17:{s:4:"name";s:6:"Bartik";s:11:"description";s:86:"A flexible, recolorable theme with many regions and a responsive, mobile-first layout.";s:7:"package";s:4:"Core";s:7:"version";s:7:"8.0-dev";s:4:"core";s:3:"8.x";s:11:"stylesheets";a:2:{s:3:"all";a:3:{s:14:"css/layout.css";s:33:"core/themes/bartik/css/layout.css";s:13:"css/style.css";s:32:"core/themes/bartik/css/style.css";s:14:"css/colors.css";s:33:"core/themes/bartik/css/colors.css";}s:5:"print";a:1:{s:13:"css/print.css";s:32:"core/themes/bartik/css/print.css";}}s:7:"regions";a:17:{s:6:"header";s:6:"Header";s:4:"help";s:4:"Help";s:8:"page_top";s:8:"Page top";s:11:"page_bottom";s:11:"Page bottom";s:11:"highlighted";s:11:"Highlighted";s:8:"featured";s:8:"Featured";s:7:"content";s:7:"Content";s:13:"sidebar_first";s:13:"Sidebar first";s:14:"sidebar_second";s:14:"Sidebar second";s:14:"triptych_first";s:14:"Triptych first";s:15:"triptych_middle";s:15:"Triptych middle";s:13:"triptych_last";s:13:"Triptych last";s:18:"footer_firstcolumn";s:19:"Footer first column";s:19:"footer_secondcolumn";s:20:"Footer second column";s:18:"footer_thirdcolumn";s:19:"Footer third column";s:19:"footer_fourthcolumn";s:20:"Footer fourth column";s:6:"footer";s:6:"Footer";}s:8:"settings";a:1:{s:20:"shortcut_module_link";s:1:"0";}s:6:"engine";s:11:"phptemplate";s:8:"features";a:9:{i:0;s:4:"logo";i:1;s:7:"favicon";i:2;s:4:"name";i:3;s:6:"slogan";i:4;s:17:"node_user_picture";i:5;s:20:"comment_user_picture";i:6;s:25:"comment_user_verification";i:7;s:9:"main_menu";i:8;s:14:"secondary_menu";}s:10:"screenshot";s:33:"core/themes/bartik/screenshot.png";s:3:"php";s:5:"5.3.5";s:7:"scripts";a:0:{}s:5:"mtime";i:1355647144;s:15:"overlay_regions";a:2:{i:0;s:7:"content";i:1;s:4:"help";}s:14:"regions_hidden";a:2:{i:0;s:8:"page_top";i:1;s:11:"page_bottom";}s:28:"overlay_supplemental_regions";a:1:{i:0;s:8:"page_top";}}s:5:"owner";s:50:"core/themes/engines/phptemplate/phptemplate.engine";s:6:"prefix";s:11:"phptemplate";s:8:"template";b:1;s:6:"status";i:1;}s:5:"seven";O:8:"stdClass":8:{s:3:"uri";s:28:"core/themes/seven/seven.info";s:8:"filename";s:28:"core/themes/seven/seven.info";s:4:"name";s:5:"seven";s:4:"info";a:17:{s:4:"name";s:5:"Seven";s:11:"description";s:65:"A simple one-column, tableless, fluid width administration theme.";s:7:"package";s:4:"Core";s:7:"version";s:7:"8.0-dev";s:4:"core";s:3:"8.x";s:11:"stylesheets";a:1:{s:6:"screen";a:1:{s:9:"style.css";s:27:"core/themes/seven/style.css";}}s:8:"settings";a:1:{s:20:"shortcut_module_link";s:1:"1";}s:7:"regions";a:5:{s:7:"content";s:7:"Content";s:4:"help";s:4:"Help";s:8:"page_top";s:8:"Page top";s:11:"page_bottom";s:11:"Page bottom";s:13:"sidebar_first";s:13:"First sidebar";}s:14:"regions_hidden";a:3:{i:0;s:13:"sidebar_first";i:1;s:8:"page_top";i:2;s:11:"page_bottom";}s:6:"engine";s:11:"phptemplate";s:8:"features";a:9:{i:0;s:4:"logo";i:1;s:7:"favicon";i:2;s:4:"name";i:3;s:6:"slogan";i:4;s:17:"node_user_picture";i:5;s:20:"comment_user_picture";i:6;s:25:"comment_user_verification";i:7;s:9:"main_menu";i:8;s:14:"secondary_menu";}s:10:"screenshot";s:32:"core/themes/seven/screenshot.png";s:3:"php";s:5:"5.3.5";s:7:"scripts";a:0:{}s:5:"mtime";i:1355647144;s:15:"overlay_regions";a:2:{i:0;s:7:"content";i:1;s:4:"help";}s:28:"overlay_supplemental_regions";a:1:{i:0;s:8:"page_top";}}s:5:"owner";s:50:"core/themes/engines/phptemplate/phptemplate.engine";s:6:"prefix";s:11:"phptemplate";s:8:"template";b:1;s:6:"status";i:1;}s:5:"stark";O:8:"stdClass":8:{s:3:"uri";s:28:"core/themes/stark/stark.info";s:8:"filename";s:28:"core/themes/stark/stark.info";s:4:"name";s:5:"stark";s:4:"info";a:16:{s:4:"name";s:5:"Stark";s:11:"description";s:208:"This theme demonstrates Drupal's default HTML markup and CSS styles. To learn how to build your own theme and override Drupal's default code, see the Theming Guide.";s:7:"package";s:4:"Core";s:7:"version";s:7:"8.0-dev";s:4:"core";s:3:"8.x";s:11:"stylesheets";a:1:{s:3:"all";a:1:{s:14:"css/layout.css";s:32:"core/themes/stark/css/layout.css";}}s:6:"engine";s:11:"phptemplate";s:7:"regions";a:9:{s:13:"sidebar_first";s:12:"Left sidebar";s:14:"sidebar_second";s:13:"Right sidebar";s:7:"content";s:7:"Content";s:6:"header";s:6:"Header";s:6:"footer";s:6:"Footer";s:11:"highlighted";s:11:"Highlighted";s:4:"help";s:4:"Help";s:8:"page_top";s:8:"Page top";s:11:"page_bottom";s:11:"Page bottom";}s:8:"features";a:9:{i:0;s:4:"logo";i:1;s:7:"favicon";i:2;s:4:"name";i:3;s:6:"slogan";i:4;s:17:"node_user_picture";i:5;s:20:"comment_user_picture";i:6;s:25:"comment_user_verification";i:7;s:9:"main_menu";i:8;s:14:"secondary_menu";}s:10:"screenshot";s:32:"core/themes/stark/screenshot.png";s:3:"php";s:5:"5.3.5";s:7:"scripts";a:0:{}s:5:"mtime";i:1355647144;s:15:"overlay_regions";a:2:{i:0;s:7:"content";i:1;s:4:"help";}s:14:"regions_hidden";a:2:{i:0;s:8:"page_top";i:1;s:11:"page_bottom";}s:28:"overlay_supplemental_regions";a:1:{i:0;s:8:"page_top";}}s:5:"owner";s:50:"core/themes/engines/phptemplate/phptemplate.engine";s:6:"prefix";s:11:"phptemplate";s:8:"template";b:1;s:6:"status";i:0;}}
)
' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php:554
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Driver/mysql/Insert.php(34): Drupal\Core\Database\Connection->query('INSERT INTO {ke...', Array, Array)
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Query/Merge.php(424): Drupal\Core\Database\Driver\mysql\Insert->execute()
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.php(95): Drupal\Core\Database\Query\Merge->execute()
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/system.module(3022): Drupal\Core\KeyValueStore\DatabaseStorage->set('system.theme.da...', Array)
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(211): system_rebuild_theme_data()
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(105): system_list('module_enabled')
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(919): module_list()
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(830): module_hook_info()
#8 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(1000): module_implements('stream_wrappers')
#9 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/file.inc(195): module_invoke_all('stream_wrappers')
#10 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/file.inc(251): file_get_stream_wrappers()
#11 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/file.inc(288): file_stream_wrapper_get_class(false)
#12 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/file.inc(503): file_stream_wrapper_valid_scheme(false)
#13 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(853): file_prepare_directory('sites/default/f...', 3)
#14 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php(649): Drupal\simpletest\TestBase->prepareEnvironment()
#15 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/field/lib/Drupal/field/Tests/FieldTestBase.php(22): Drupal\simpletest\WebTestBase->setUp()
#16 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(696): Drupal\field\Tests\FieldTestBase->setUp()
#17 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(381): Drupal\simpletest\TestBase->run()
#18 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('183', 'Drupal\field\Te...')
#19 {main}FATAL Drupal\field\Tests\FieldInfoTest: test runner returned a non-zero error code (1).

I've also archived the results here: http://www.webcitation.org/6CxMBOtNt

This should probably be investigated but in the mean time I'm going to run the tests again.

#40: 1821548-import-diff-40.patch queued for re-testing.

@Dean_Reilly
This happens currently a lot on every kind of issue, so don't worry.

I talked to nod_ on IRC today and he said the width issue should be addressed in #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases if you want to help out there.

I was playing with this because I thought we could nowrap the td's to force the dialog wider as a stopgap measure, but that doesn't work (it just adds a scrollbar.) However while looking at that I did notice some things with the css

+++ b/core/modules/system/system.diff.cssundefined
@@ -0,0 +1,86 @@
+table.diff {
+  border-spacing: 4px;
+  margin-bottom: 20px;
+  table-layout: fixed;
+  width: 100%;
+}

It looks like we're not actually adding the 'diff' class to the table, so a ton of the css here isn't getting picked up.

+++ b/core/modules/system/system.diff.cssundefined
@@ -0,0 +1,86 @@
+td.diff-prevlink {
+  text-align: left;
+}
+td.diff-nextlink {
+  text-align: right;
+}

These don't appear to be used at all.

+++ b/core/modules/system/system.diff.cssundefined
@@ -0,0 +1,86 @@
+/**
+ * Inline diff metadata
+ */
+.diff-inline-metadata {
+  padding:4px;
+  border:1px solid #ddd;
+  background:#fff;
+  margin:0px 0px 10px;
+}
+
+.diff-inline-legend { font-size:11px; }
+
+.diff-inline-legend span,
+.diff-inline-legend label { margin-right:5px; }

nor these

fyi, we have been punting the modal in most issues so far. That's why we dont have any in core yet :)

It's pretty useless as it is without the other patch too, the diffs shoved into a very tiny window and are impossible to read. So I'll postpone until the other lands.

If I'm understanding this issue correctly, the only part of #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases that this is blocked on is being able to customize the dialog size. Therefore, to help get this unblocked faster, I spun off that portion to #1909144: Allow #ajax['dialog'] to contain options other than 'modal'.

fyi, we have been punting the modal in most issues so far

@Bojhan: can you link to the issues waiting on modal improvements? I'd like to review them to see which can be unblocked by #1909144: Allow #ajax['dialog'] to contain options other than 'modal' vs. which need the rest of #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases.

Current patch no longer applies, here is a reroll. All it does is move config_diff()

It appears that #1909144: Allow #ajax['dialog'] to contain options other than 'modal' is about to land, and once it does adding a static width to make this readable will be very easy (I just tested it and it works well.) Is that the best answer though? I'm mostly wondering about what to do about mobile devices and the like. I think its fair to argue that for this specific screen that is probably not a huge concern (I don't see a lot of people doing config imports from their phone) but its something I wanted to think about and I'm not sure what best practices are at this moment.

Once that issue lands, all of the options documented in http://api.jqueryui.com/dialog/ will work, not just width. Perhaps using dialogClass would be better, and include CSS to style it as desired.

OK #1909144: Allow #ajax['dialog'] to contain options other than 'modal' has landed so un-postponing this. I'm happy just going with a hardcoded width of like 700px as a start, but if someone wants to take a shot at making something a little more robust, feel free. This is not really my strong area.

Well here's a patch that just makes it 700px wide, which I think is enough to just get this in.

Correct me if I'm wrong, but I think this issue is correctly classified as a task, so there's no feature freeze rush to get it in before it's fully ready. Given that, here's some feedback.

+++ b/core/includes/config.inc
@@ -324,3 +325,45 @@ function config_get_entity_type_by_name($name) {
+function config_diff(StorageInterface $source_storage, StorageInterface $target_storage, $name) {

We need a "unit" test for this function. I put "unit" in quotes, because the theme('table') makes this not strictly unit testable, but whatever base test class we need to use, we need to test the output of this function for when there's no difference, and when there's 2 or more lines of difference.

+++ b/core/includes/theme.inc
@@ -3303,5 +3303,36 @@ function drupal_common_theme() {
+    'diff_header_line' => array(
+      'variables' => array('lineno' => NULL),
+    ),
+    'diff_content_line' => array(
+      'variables' => array('line' => NULL),
+    ),
+    'diff_empty_line' => array(
+      'variables' => array('line' => NULL),
+    ),
+
   );
 }
+
+/**
+ * Theme function for a header line in the diff.
+ */
+function theme_diff_header_line($vars) {
+  return '<strong>' . t('Line %lineno', array('%lineno' => $vars['lineno'])) . '</strong>';
+}
+
+/**
+ * Theme function for a content line in the diff.
+ */
+function theme_diff_content_line($vars) {
+  return '<div>' . $vars['line'] . '</div>';
+}
+
+/**
+ * Theme function for an empty line in the diff.
+ */
+function theme_diff_empty_line($vars) {
+  return $vars['line'];

This patch removes all calls to these, so why are we adding them?

+++ b/core/modules/config/config.admin.inc
@@ -61,10 +62,21 @@ function config_admin_sync_form(array &$form, array &$form_state, StorageInterfa
+            '#ajax' => array('dialog' => array('modal' =>TRUE, 'width' => '700px')),

I opened #1918744: Define some dialog CSS classes for common dialog sizes as a follow up, so the 700px doesn't need to hold up this issue.

+++ b/core/modules/config/config.module
@@ -48,6 +48,14 @@ function config_menu() {
+  $items['admin/config/development/sync/diff/%'] = array(

We also need a web test to this URL. The web test doesn't need to ask for it in a modal: it can just make a regular request, which would result in the same thing a user with JS disabled would trigger.

+++ b/core/modules/system/system.diff.css
@@ -0,0 +1,86 @@
+html.js .diff-js-hidden { display: none; }

Are there any elements using this class? I couldn't find any.

The attached patch does the following:

- Splits display logic from functional logic in config_diff()
- Adds a unit test for Diff functionality
- Adds an incomplete UI test for diff functionality
- Removes unused theme functions and css

I would like some guidance on the UI tests. Right now they don't actually test anything because I'm not quite sure what the best way to detect specific types of diffs in the UI is, particularly if HTML is interspersed in the text you're looking for. Also the UI test replicates a lot of functionality from the unit test and I'm wondering if there is a good place to put that common code. I didn't see anything obvious since one descends from DrupalUnitTestBase and the other from WebTestBase.

One bug discovered from Diff 7.x-3.x there was that td.diff-context style needs to be defined before td.diff-deletedline, td.diff-addedline, and td.diff-diffchange.

i.e This class should be added to all line items by DrupalDiffFormatter for global styling of these cells, and when defined last in system.diff.css, it will override the other line styles, namely the background colors.

Adding fix for:
#64.2 - Ref attached screen 2 (on click, getting loading icon)
#64.3
#64.4 - Seems it was already fixed and just changing the order of style elements fixed this the problem.
#64.7 - Already in h3, but some reason it is not getting rendered proper:

+++ b/core/modules/config/config.admin.incundefined
@@ -116,3 +131,54 @@ function config_admin_import_form_submit($form, &$form_state) {
+  $output['title'] = array(
+    '#theme' => 'html_tag',
+    '#tag' => 'h3',
+    '#value' => t('View changes of @config_file', array('@config_file' => $config_file)),

Link to operations change:

Link to operations change(loading):

Diff with color:

Adding screenshots..

#65: 1821548-import-diff-65.patch queued for re-testing.

Confirmed patch in #65 addresses:

• #64.1
• #64.2
• #64.3
• #64.4
• #64.7

Which leaves:

• #64.5 and .6, although not sure if those are caused by this patch
• #64.8 diff jumps to bottom instead of top
• #64.9 diff displays 'false' for empty config

Here's a patch that attempts to address the new/removed config by changing the array('false') to array(t('File removed')) (or added) prior to passing the configs into Diff.

Setting back to needs review, because I'm not sure 64 (5,6 and 8) are caused by this.

Adding a file isn't working as expected.

To test adding a file, I did this:

1. Clone "frontpage" view through the UI, as "my_frontpage"
2. Make some edits and save my_frontpage
3. from config's "active" directory:
   

   cp views.view.my_frontpage.yml ../staging
   cp manifest.views.view.yml ../staging
   rm views.view.my_frontpage.yml
   vim manifest.views.view.yml # remove following two lines:
   my_frontpage:
     name: views.view.my_frontpage
   

4. drush cc all
5. Load up the configuration sync page again

When I remove a file - in this case a view, via deleting its two lines from the views manifest - the result looks OK:

Found the bug -- a === used instead of a = in the last patch.

New patch fixes that and also does not include "config.inc.orig."

64.5 and 64.6 are definitely not covered by this patch, but I opened followups

#1938504: Close button on modal dialogs is off-center when not being hovered over
#1938508: Close button on modal dialogs is styled differently than the close button in the overlay

As far as #8, webchick indicated to me on IRC that she was fine with that being a followup as well.

#1938514: Diff dialog jumps to bottom when opened

So I'm back to RTBC on this. Thanks everyone!

Changing this back to needs work, as the commit 80fd0f970ca for the issue #1843220: Convert form AJAX to new AJAX API seems to have broken the dialog.

same here, ajax dialog don't work, but dialog is working (if I open it mannualy in a new tab)

Yeah, we broke dialogs with that AJAX patch...we'll fix it up.

Back to fixed I guess

Tagging.

In comment #49, @heyrocker lists classes that are not found to be used in core (i.e. .diff-inline-legend), but yet these classes got rolled into the patch that was committed. I was working on issue #2396473: Add missing RTL rules to System CSS and in trying to test that patch, I couldn't find .diff-inline-legend anywhere. Shouldn't the classes listed by @heyrocker be removed from system.diff.css?