Postponed on #1697256: Create a UI for importing new configuration.

In reviewing the CMI UI patch, I came across a situation where the configuration I thought I was moving to production was not in fact the configuration showing up in the import UI as what was about to be imported. See #1697256-86: Create a UI for importing new configuration. Screenshot:

Two things awaiting import, instead of one.

Despite the fact that I had only changed the site maintenance message (system.maintenance), it also thought it should move over system.cron. Turns out this is a bug (#1821530: Move cron key from configuration to state system), but had I not had Git at my disposal to tell what the heck was going on (which we should not require of our users IMO), I would've been stopped dead in my tracks, unsure how to proceed.

Even despite this bug, any one of these keys might have 7000 things underneath them. Another example of this is at the top of that comment, when I intended to add a site slogan, but what actually happened was it changed the site slogan and the site name both. This is another bug (#1763640: Introduce config context to make original config and different overrides accessible) but it's completely conceivable that such a situation could arise in the "real world" as well.

So I really think it's necessary to have a means of "previewing" the configuration system changes before I import them and blow away my existing config. I had suggested to heyrocker as an MVP just something like:

system.cron
- cron_key
system.maintenace
- maintenance_message

Though he pointed out that incorporating a Diff library of some kind and just calling a function would likely be less work/overhead.

In any case, I think the UI is rather unusable without this, so filing as a major follow-up task once the UI is in.

Files: 
CommentFileSizeAuthor
#71 1821548-import-diff-71.patch15.69 KBadamdicarlo
PASSED: [[SimpleTest]]: [MySQL] 52,919 pass(es). View
#71 1821548-difftxt-69-71.txt16.99 KBadamdicarlo
#70 1821548-d8-config-sync-diff-removed-file.png39.71 KBadamdicarlo
#70 1821548-d8-config-sync-diff-added-file.png23.92 KBadamdicarlo
#69 1821548-import-diff-69.patch31.37 KBjhedstrom
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#69 1821548-difftxt-65-69.txt762 bytesjhedstrom
#65 1821548-import-diff-65.patch15.43 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,264 pass(es). View
#65 1821548-difftxt-62-65.txt2.05 KBvijaycs85
#66 diff-2.png26.18 KBvijaycs85
#66 diff-1.png14.73 KBvijaycs85
#66 diff-1.1.png17.5 KBvijaycs85
#64 Screen Shot 2013-02-18 at 8.38.20 PM.png57.3 KBwebchick
#64 Screen Shot 2013-02-18 at 8.44.19 PM.png49.6 KBwebchick
#64 Screen Shot 2013-02-18 at 8.48.34 PM.png69.25 KBwebchick
#62 1821548-import-diff-62.patch15.35 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 51,310 pass(es). View
#60 1821548-import-diff-60.patch10.12 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 50,314 pass(es). View
#56 1821548-import-diff-56.patch10.1 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 50,273 pass(es). View
#40 1821548-import-diff-40.patch10.17 KBDean Reilly
PASSED: [[SimpleTest]]: [MySQL] 49,340 pass(es). View
#40 1821548-interdiff-33-40.txt4.5 KBDean Reilly
#40 1821548-interdiff-37-40.txt763 bytesDean Reilly
#37 import_diff-1821548-37.patch8.3 KBDean Reilly
PASSED: [[SimpleTest]]: [MySQL] 49,312 pass(es). View
#37 interdiff.txt4.58 KBDean Reilly
#38 sync-page.png201.31 KBDean Reilly
#38 diff-modal.png200.1 KBDean Reilly
#33 1821548-import-diff-33.patch8.56 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 49,254 pass(es). View
#33 1821548-interdiff-31-33.txt1.77 KBheyrocker
#31 1821548-31.patch6.79 KBswentel
None View
#31 interdiff.txt2.17 KBswentel
#26 1821548-26.patch6.86 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View
#15 1821548-15.drupal8.config-diff.patch46.53 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 48,122 pass(es). View
#13 collapssed.png17.32 KBdawehner
#13 open.png24.21 KBdawehner
#13 interdiff.txt2.78 KBdawehner
#13 drupal-1821548-11.patch65.51 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 48,067 pass(es). View
#11 Synchronize configuration drupal.local-122508.png128.39 KBheyrocker
#11 1821548-import-diff-11.patch64.79 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 48,055 pass(es). View
#10 1821548-import-diff-10.patch65.82 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 48,063 pass(es). View
#7 Screen Shot 2012-11-01 at 11.32.15 PM.png32.03 KBheyrocker
#7 Screen Shot 2012-10-14 at 2.34.28 PM.png39.08 KBheyrocker

Comments

mitchell’s picture

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.

mitchell’s picture

Issue tags: +revision

tagging

heyrocker’s picture

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+).

webchick’s picture

Agreed w/ heyrocker.

mitchell’s picture

Status: Postponed » Active

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

heyrocker’s picture

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.

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.

heyrocker’s picture

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.

Screen Shot 2012-11-01 at 11.32.15 PM.png

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

For reference here is the config import screen

Screen Shot 2012-10-14 at 2.34.28 PM.png

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.

Bojhan’s picture

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.

heyrocker’s picture

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.

heyrocker’s picture

Status: Active » Needs review
FileSize
65.82 KB
PASSED: [[SimpleTest]]: [MySQL] 48,063 pass(es). View

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.

heyrocker’s picture

FileSize
64.79 KB
PASSED: [[SimpleTest]]: [MySQL] 48,055 pass(es). View
128.39 KB

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.

Synchronize configuration   drupal.local-122508.png

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.

heyrocker’s picture

dawehner’s picture

Issue tags: +D8UX usability
FileSize
65.51 KB
PASSED: [[SimpleTest]]: [MySQL] 48,067 pass(es). View
2.78 KB
24.21 KB
17.32 KB

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

Let's hide the difference by default.

dawehner’s picture

Issue tags: -D8UX usability

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

alexpott’s picture

Issue tags: +Needs tests
FileSize
46.53 KB
PASSED: [[SimpleTest]]: [MySQL] 48,122 pass(es). View

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.

alexpott’s picture

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

moshe weitzman’s picture

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.

heyrocker’s picture

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.

heyrocker’s picture

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.

sun’s picture

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

heyrocker’s picture

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.

YesCT’s picture

Status: Needs review » Needs work

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

Bojhan’s picture

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

webchick’s picture

Well, given we don't have a modal yet, and given that the UI is... while not exactly useless, let's call it "highly unusable" without this, I think it's worth putting this in as a stop-gap and then adding modals when/if we have them.

Bojhan’s picture

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

swentel’s picture

Status: Needs work » Needs review
FileSize
6.86 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View

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

Status: Needs review » Needs work

The last submitted patch, 1821548-26.patch, failed testing.

swentel’s picture

sun’s picture

Status: Needs work » Needs review

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

sun’s picture

+++ 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).

swentel’s picture

Issue tags: -Needs tests, -revision +revision control
FileSize
2.17 KB
6.79 KB
None View

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

heyrocker’s picture

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

heyrocker’s picture

FileSize
1.77 KB
8.56 KB
PASSED: [[SimpleTest]]: [MySQL] 49,254 pass(es). View

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.

Status: Needs review » Needs work

The last submitted patch, 1821548-import-diff-33.patch, failed testing.

Bojhan’s picture

We have modals.

Dean Reilly’s picture

Status: Needs work » Needs review

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

Dean Reilly’s picture

FileSize
4.58 KB
8.3 KB
PASSED: [[SimpleTest]]: [MySQL] 49,312 pass(es). View

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.
Dean Reilly’s picture

FileSize
200.1 KB
201.31 KB

A couple of screenshots.

heyrocker’s picture

Status: Needs review » Needs work

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?

Dean Reilly’s picture

FileSize
763 bytes
4.5 KB
10.17 KB
PASSED: [[SimpleTest]]: [MySQL] 49,340 pass(es). View

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

YesCT’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Needs tests, -revision, -Configuration system

The last submitted patch, 1821548-import-diff-40.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Needs tests, +revision, +Configuration system

The last submitted patch, 1821548-import-diff-40.patch, failed testing.

Dean Reilly’s picture

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.

Dean Reilly’s picture

Status: Needs work » Needs review

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

dawehner’s picture

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

heyrocker’s picture

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.

heyrocker’s picture

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

webchick’s picture

I was told this is blocked on the dialog patch at #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases. It seems like we should either punt the modals to a follow-up or we should mark this postponed. My preference would be the former, because we've had a not-very-useful UI in core for months now because of this missing feature.

Bojhan’s picture

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

webchick’s picture

Yes, well, we need to make progress in the meantime while we wait for the modal to be actually useful, IMO.

#35 says "we have modals", but according to #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases that doesn't seem to actually be true.

heyrocker’s picture

Status: Needs review » Postponed

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.

effulgentsia’s picture

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

effulgentsia’s picture

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.

heyrocker’s picture

FileSize
10.1 KB
PASSED: [[SimpleTest]]: [MySQL] 50,273 pass(es). View

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

heyrocker’s picture

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.

effulgentsia’s picture

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.

heyrocker’s picture

Status: Postponed » Active

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.

heyrocker’s picture

Status: Active » Needs review
FileSize
10.12 KB
PASSED: [[SimpleTest]]: [MySQL] 50,314 pass(es). View

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

effulgentsia’s picture

Status: Needs review » Needs work

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.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
15.35 KB
PASSED: [[SimpleTest]]: [MySQL] 51,310 pass(es). View

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.

Alan D.’s picture

Status: Needs review » Needs work

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.

webchick’s picture

Here are some screenshots.

A new, changed, and removed file.

1) It seems like those operations columns ought to line up with one another.
2) I believe the standard is to use the dropbutton pattern everywhere in core for operations, even if there's only one option. Tim Plunkett confirmed this on IRC.
3) Operations should be verbs, so I'd call this link "View differences" or something.

Simple diff:

A simple diff with two small changes

4) The styling is a bit ugly. :( Any way to do something more like https://github.com/josh-/slate/commit/52606363980eed0d96593f7fe55c58e7fd... ?
5) The "x" icon moves around when I hover over it. I'm assuming that isn't introduced by this patch.
6) The "x" icon is different from the Overlay icon, which is odd. I'm assuming that isn't introduced by this patch.
7) "Configuration file diff" title is in a span? Why is this not a heading 1/2? (This also might not be introduced by this patch.)

Bigger diff:

Bigger diff

8) This inexplicably opens off-centre and scrolled to the bottom. It would make more sense for it to be at the top, and for the dialog to be centred in the middle.
9) Upon adding/removing a file, the changed side just says "false". Not sure if we can make that something more explicit like "No file found."

vijaycs85’s picture

FileSize
2.05 KB
15.43 KB
PASSED: [[SimpleTest]]: [MySQL] 52,264 pass(es). View

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:
diff-1.png
Link to operations change(loading):
diff-1.1.png
Diff with color:
diff-2.png

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
17.5 KB
14.73 KB
26.18 KB

Adding screenshots..

vijaycs85’s picture

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

jhedstrom’s picture

Status: Needs review » Needs work

Confirmed patch in #65 addresses:

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
jhedstrom’s picture

Status: Needs work » Needs review
FileSize
762 bytes
31.37 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

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.

adamdicarlo’s picture

Status: Needs review » Needs work
FileSize
23.92 KB
39.71 KB

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

screenshot of diff file file added

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

adamdicarlo’s picture

Status: Needs work » Needs review
FileSize
16.99 KB
15.69 KB
PASSED: [[SimpleTest]]: [MySQL] 52,919 pass(es). View

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

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

heyrocker’s picture

Status: Needs review » Reviewed & tested by the community

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!

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +SprintWeekend2013

Awesome. I don't have a great environment for testing this by hand right now, but the screenshots look great and this has been outstanding for months now, so I'm happy to get something in and iterate on it later. No better way to test this UI than to get it in front of new D8 contributors during #SprintWeekend. ;)

Committed and pushed to 8.x. WOOHOO!

jessehs’s picture

Status: Fixed » Needs work

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.

ybabel’s picture

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

mkadin’s picture

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

aspilicious’s picture

Status: Needs work » Fixed

Back to fixed I guess

jibran’s picture

Issue tags: +modal dialog

Tagging.

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

pbland’s picture

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?