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:
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.
Comment | File | Size | Author |
---|---|---|---|
#71 | 1821548-import-diff-71.patch | 15.69 KB | adamdicarlo |
#71 | 1821548-difftxt-69-71.txt | 16.99 KB | adamdicarlo |
#70 | 1821548-d8-config-sync-diff-removed-file.png | 39.71 KB | adamdicarlo |
#70 | 1821548-d8-config-sync-diff-added-file.png | 23.92 KB | adamdicarlo |
#69 | 1821548-import-diff-69.patch | 31.37 KB | jhedstrom |
Comments
Comment #1
mitchell CreditAttribution: mitchell commentedWould 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.
Comment #2
mitchell CreditAttribution: mitchell commentedtagging
Comment #3
gddI 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+).
Comment #4
webchickAgreed w/ heyrocker.
Comment #5
mitchell CreditAttribution: mitchell commentedI agree that a native PHP library is needed. Please evaluate: #1826156: Move to Text_Diff library.
Comment #6
gddI 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.
Comment #7
gddI 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.
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.
Comment #8
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #9
gddAnother 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.
Comment #10
gddHere'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.
Comment #11
gddHere'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.
Comment #12
gddRelated: #120955: Integrate Diff into Core
Comment #13
dawehnerFor consistency i think it makes sense to show created/deleting config as well.
Let's hide the difference by default.
Comment #14
dawehnerCouldn't we just create the renderer from outsite optionally.
Comment #15
alexpottI'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.
Comment #16
alexpottTranscript 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
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #18
gddMy 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.
Comment #19
gddSpent 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
This should be removed since the options parameter is gone now.
Since we're showing diffs for deleted and inserted now, this css should just be included all the time now.
I liked the 'New' and 'Old' headers we had before and they would be easy to add back in.
Comment #20
sunThe 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.)
Comment #21
gddWell, 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.
Comment #22
YesCT CreditAttribution: YesCT commented#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.
Comment #23
Bojhan CreditAttribution: Bojhan commentedI don't think the proposed UI makes much sense, the diff should be in a modal..
Comment #24
webchickWell, 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.
Comment #25
Bojhan CreditAttribution: Bojhan commented@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.
Comment #26
swentel CreditAttribution: swentel commentedNew patch now the engine is in, with the nitpicks fixed from #19
Comment #28
swentel CreditAttribution: swentel commentedFailing tests is going to be fixed by #1847828: [HEAD BROKEN] DrupalUnitTestBase and this container and transliteration and kernel and whatnot
Comment #29
sun#26: 1821548-26.patch queued for re-testing.
Comment #30
sunI 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."
We can use [] instead of array() here.
Let's add a blank line between the switch cases to clarify that these are not fall-through situations.
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).
Comment #31
swentel CreditAttribution: swentel commentedSomething like this ? I'd would also argue that we can write tests when we're actually refactoring the Diffengine class, no ?
Comment #32
gdd#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.
Comment #33
gddThe 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.
Comment #35
Bojhan CreditAttribution: Bojhan commentedWe have modals.
Comment #36
Dean Reilly CreditAttribution: Dean Reilly commented#33: 1821548-import-diff-33.patch queued for re-testing.
Comment #37
Dean Reilly CreditAttribution: Dean Reilly commentedHere's a go at switching to dialogs api. A couple of issues still need to be resolved:
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.
Again this seems like it's going to be a common problem and so should probably be dealt with in another issue.
Comment #38
Dean Reilly CreditAttribution: Dean Reilly commentedA couple of screenshots.
Comment #39
gddHmmm, 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?
Comment #40
Dean Reilly CreditAttribution: Dean Reilly commentedOops 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
Comment #41
YesCT CreditAttribution: YesCT commentedComment #43
vijaycs85#40: 1821548-import-diff-40.patch queued for re-testing.
Comment #45
Dean Reilly CreditAttribution: Dean Reilly commentedInteresting. I don't think this failure is related to this patch. This is the trace:
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.
Comment #46
Dean Reilly CreditAttribution: Dean Reilly commented#40: 1821548-import-diff-40.patch queued for re-testing.
Comment #47
dawehner@Dean_Reilly
This happens currently a lot on every kind of issue, so don't worry.
Comment #48
gddI 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.
Comment #49
gddI 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
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.
These don't appear to be used at all.
nor these
Comment #50
webchickI 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.
Comment #51
Bojhan CreditAttribution: Bojhan commentedfyi, we have been punting the modal in most issues so far. That's why we dont have any in core yet :)
Comment #52
webchickYes, 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.
Comment #53
gddIt'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.
Comment #54
effulgentsia CreditAttribution: effulgentsia commentedIf 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'.
Comment #55
effulgentsia CreditAttribution: effulgentsia commented@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.
Comment #56
gddCurrent patch no longer applies, here is a reroll. All it does is move config_diff()
Comment #57
gddIt 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.
Comment #58
effulgentsia CreditAttribution: effulgentsia commentedOnce 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.Comment #59
gddOK #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.
Comment #60
gddWell here's a patch that just makes it 700px wide, which I think is enough to just get this in.
Comment #61
effulgentsia CreditAttribution: effulgentsia commentedCorrect 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.
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.
This patch removes all calls to these, so why are we adding them?
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.
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.
Are there any elements using this class? I couldn't find any.
Comment #62
gddThe 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.
Comment #63
Alan D. CreditAttribution: Alan D. commentedOne 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.
Comment #64
webchickHere are some screenshots.
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:
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:
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."
Comment #65
vijaycs85Adding 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:
Link to operations change:
Link to operations change(loading):
Diff with color:
Comment #66
vijaycs85Adding screenshots..
Comment #67
vijaycs85#65: 1821548-import-diff-65.patch queued for re-testing.
Comment #68
jhedstromConfirmed patch in #65 addresses:
Which leaves:
Comment #69
jhedstromHere's a patch that attempts to address the new/removed config by changing the
array('false')
toarray(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.
Comment #70
adamdicarlo CreditAttribution: adamdicarlo commentedAdding a file isn't working as expected.
To test adding a file, I did this:
drush cc all
When I remove a file - in this case a view, via deleting its two lines from the views manifest - the result looks OK:
Comment #71
adamdicarlo CreditAttribution: adamdicarlo commentedFound the bug -- a === used instead of a = in the last patch.
New patch fixes that and also does not include "config.inc.orig."
Comment #72
gdd64.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!
Comment #73
webchickAwesome. 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!
Comment #74
jessehsChanging 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.
Comment #75
ybabel CreditAttribution: ybabel commentedsame here, ajax dialog don't work, but dialog is working (if I open it mannualy in a new tab)
Comment #76
mkadin CreditAttribution: mkadin commentedYeah, we broke dialogs with that AJAX patch...we'll fix it up.
Comment #77
aspilicious CreditAttribution: aspilicious commentedBack to fixed I guess
Comment #78
jibranTagging.
Comment #80
pbland CreditAttribution: pbland commentedIn 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?