Updated: Comment #N

Problem/Motivation

So in porting http://drupal.org/project/pants module, naturally I added views integration for my "Recent pants" block, because duh! ;) I spent a good 20 minutes clicking together something totally awesome in the UI, and now I want to move it into my module as a default view.

In the past this would be an "Export" operation off the main page dropbutton, but of course that's not there because CMI exists. However, the workflow for getting this out of CMI and into my module is not totally trivial. I have to do digging around in my sites/default/files/config_jhGjasasduad directory.

I can see this being a "won't fix, you're a developer, so suck it up" which would be fair enough, but it's a bit of a DX loss, so figured I'd start a discussion about it.

Proposed resolution

Introduce a new controller which provides a generic export for, which is then used by views and is accessible from the config entity listing.

User interface changes

Added a export configuration/link.


Files: 
CommentFileSizeAuthor
#57 config-1898794-57.patch8.08 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,921 pass(es). View
#55 interdiff.txt655 bytestim.plunkett
#55 config_entity-export-1898794-55-A.patch23.46 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 59,227 pass(es), 2 fail(s), and 0 exception(s). View
#55 config_entity-export-1898794-55-B.patch16.41 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,280 pass(es). View
#55 config_entity-export-1898794-55-C.patch7.95 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,193 pass(es). View
#52 config_entity-export-1898794-52.patch23.47 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 58,833 pass(es), 2 fail(s), and 13 exception(s). View
#48 export.png44.22 KBdawehner
#47 interdiff.txt680 bytesdawehner
#47 config_entity-export-1898794-47.patch23.47 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,948 pass(es). View
#44 Screen Shot 2013-09-10 at 8.32.39 PM.png36.3 KBtim.plunkett
#44 interdiff.txt769 bytestim.plunkett
#44 config-entity-1898794-44.patch24.44 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,174 pass(es). View
#38 config-export-1898794-38.patch24.23 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,853 pass(es). View
#38 interdiff.txt1.89 KBtim.plunkett
#36 config-entity-1898794-36.patch24.24 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,900 pass(es). View
#36 interdiff.txt1.01 KBtim.plunkett
#36 Screen Shot 2013-09-06 at 11.50.07 AM.png22.68 KBtim.plunkett
#36 Screen Shot 2013-09-06 at 11.50.17 AM.png19.85 KBtim.plunkett
#31 config-entity-1898794-31.patch24.21 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,438 pass(es). View
#28 config-entity-1898794-28.patch24.19 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,224 pass(es). View
#28 interdiff.txt2.03 KBtim.plunkett
#26 config-export-1898794-26.patch23.22 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,457 pass(es), 6 fail(s), and 2 exception(s). View
#26 interdiff.txt5.27 KBtim.plunkett
#23 config-entity-1898794-23.patch25.15 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,199 pass(es). View
#21 config-entity-1898794-21.patch25.2 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,897 pass(es). View
#21 interdiff.txt1.35 KBtim.plunkett
#19 config-export-1898794-19.patch25.2 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,967 pass(es). View
#19 interdiff.txt2.91 KBtim.plunkett
#17 config-entity-1898794-17.patch24.86 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,997 pass(es). View
#17 interdiff.txt2.59 KBtim.plunkett
#15 config-entity-1898794-15.patch23.7 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,054 pass(es), 2 fail(s), and 0 exception(s). View
#15 interdiff.txt6.47 KBtim.plunkett
#8 config-entity-1898794-8.patch17.91 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,087 pass(es), 1 fail(s), and 0 exception(s). View
#3 export.png11.22 KBdawehner
#3 export2.png36.34 KBdawehner
#3 drupal-1898794-3.patch4.55 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 49,302 pass(es). View

Comments

dawehner’s picture

Personally I really think this is quite important if we want to have at least some way of issue-queue-maintainability.

One question is whether we want to expose the actual path to the config file in the UI, or just provide an export. Are there security problems when you have the full path to a config file? It could be problematic when you can access all of them.

tim.plunkett’s picture

Honestly, having a display exactly like in D7 (a textarea you can click in, select all, copy) would be ideal.

Not everyone will have file system access, and as a module author it'll even make it easier to click together a view, and then copy it to make a mymodule/config/views.view.mymodule_view.yml file out of it.

dawehner’s picture

Status: Active » Needs review
FileSize
4.55 KB
PASSED: [[SimpleTest]]: [MySQL] 49,302 pass(es). View
36.34 KB
11.22 KB
moshe weitzman’s picture

Status: Needs review » Needs work
Issue tags: +Configuration system

I agree that this would be nice. Even nicer would be a textarea foe importing your config, like Views for D7. I looked around and did not see an issue for the import part.

Setting to CNW because everything changed :)

moshe weitzman’s picture

Title: Restore some form of 'export' functionality to views (or config entities generallyl?) » Restore export UI to views (and config entities generally)
Component: views.module » configuration entity system

Retitle and refile against config entity system

beejeebus’s picture

oh, this is a nice idea, exporting config thingies should be easy.

don't think i like putting the import a config thingie in the same issue, however.

that's nowhere near as simple, so should be it's own issue.

moshe weitzman’s picture

No interest in the export part of this?

tim.plunkett’s picture

Component: configuration entity system » views_ui.module
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
17.91 KB
FAILED: [[SimpleTest]]: [MySQL] 57,087 pass(es), 1 fail(s), and 0 exception(s). View

This was ooollllddddd, no interdiff possible.

I didn't tiptoe around here, I just did it generic from the start.
I also had to fix the views breadcrumbs, the export page's were wrong.

Needs tests.

tim.plunkett’s picture

Component: views_ui.module » configuration entity system

Oopps.

Status: Needs review » Needs work

The last submitted patch, config-entity-1898794-8.patch, failed testing.

dawehner’s picture

+++ b/core/modules/views_ui/views_ui.moduleundefined
@@ -92,6 +103,26 @@ function views_ui_menu() {
+function views_ui_edit_page_title(ViewStorageInterface $view) {
+  $name = $view->label();
+  $data = Views::viewsData()->get($view->get('base_table'));
+  if (isset($data['table']['base']['title'])) {
+    $name .= ' (' . $data['table']['base']['title'] . ')';
+  }
+  return $name;

Are you sure we want to have this full information as part of the breadcrumb?

moshe weitzman’s picture

I think a long unambigous title is fine, personally.

dawehner’s picture

Title: Restore export UI to views (and config entities generally) » Allow config entities to be exported. (Resolve regression from views in Drupal 7).
tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Rerolling, adding tests

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests +Configurables
FileSize
6.47 KB
23.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,054 pass(es), 2 fail(s), and 0 exception(s). View

The long title was in D7.

Status: Needs review » Needs work

The last submitted patch, config-entity-1898794-15.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.59 KB
24.86 KB
PASSED: [[SimpleTest]]: [MySQL] 57,997 pass(es). View

Fixed the test, and changed some comments according to beejeebus's feedback.

dawehner’s picture

@@ -32,7 +32,7 @@
+class ConfigStorageController extends EntityStorageControllerBase implements ConfigStorageControllerInterface {

The diff of this class could have been dramatically reduced.

@@ -0,0 +1,101 @@
+   *   'views.view.archive'.
...
+   *   The config prefix of the configuration entity. E.g. 'views.view'
...
+   *   The full configuration prefix, for example 'views.view.'.

Yeah snick in views everywhere!

@@ -0,0 +1,59 @@
+    $expected_export[] = 'id: dotted.default';
+    $expected_export[] = "uuid: $uuid";
+    $expected_export[] = 'label: Default';
+    $expected_export[] = 'weight: \'0\'';
+    $expected_export[] = 'style: \'\'';
+    $expected_export[] = 'status: \'1\'';
+    $expected_export[] = 'langcode: en';
+    $expected_export[] = 'protected_property: Default';
+    $expected_export[] = '';
+    $expected_export = implode("\n", $expected_export);

Any reason to not use the Yaml component to produce the expected output?

@@ -0,0 +1,35 @@
+ * Contains Drupal\config_test\ConfigTestListController.

Missing "\"

@@ -234,14 +234,6 @@ public function autocompleteTag(Request $request) {
-    $name = $view->label();
-    $data = $this->viewsData->get($view->get('base_table'));
...
-    if (isset($data['table']['base']['title'])) {
-      $name .= ' (' . $data['table']['base']['title'] . ')';
-    }
-    drupal_set_title($name);

@@ -47,6 +48,8 @@ function views_ui_menu() {
+    'title callback' => 'views_ui_edit_page_title',
+    'title arguments' => array(4),

Urgs

tim.plunkett’s picture

Issue tags: +DX (Developer Experience)
FileSize
2.91 KB
25.2 KB
PASSED: [[SimpleTest]]: [MySQL] 57,967 pass(es). View

I had to add ConfigStorageControllerInterface, and I didn't want to do it wrong :(

Yes, using the Yaml component is much better. I also realized that since we're supposed to be providing UUIDs for default config, we can hardcode that here.

dawehner’s picture

Thank you!

@@ -2,5 +2,6 @@ id: dotted.default
+uuid: 0c601ffc-6a83-49cc-ac00-a19a564aab06

Let's use 76696577-7364-7572-7061-6c636f726512 instead.

tim.plunkett’s picture

FileSize
1.35 KB
25.2 KB
PASSED: [[SimpleTest]]: [MySQL] 57,897 pass(es). View

You're absolutely right.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Added an issue summary so this is ready to fly now.

tim.plunkett’s picture

FileSize
25.15 KB
PASSED: [[SimpleTest]]: [MySQL] 58,199 pass(es). View

Rerolled for the Plugin/Core/Entity move

damiankloip’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
    @@ -536,4 +494,14 @@ public function importDelete($name, Config $new_config, Config $old_config) {
    +    $prefix = $this->getConfigPrefix();
    

    Do we need to assign this variable?

  2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityExportFormTest.php
    @@ -0,0 +1,57 @@
    +    $this->assertTrue(!empty($element), 'Textarea with config export found.');
    ...
    +    $this->assertEqual((string) $element[0], $export);
    

    Can't we just use assertFieldByXpath() here instead?

  3. +++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestListController.php
    @@ -0,0 +1,35 @@
    +  public function getOperations(EntityInterface $entity) {
    +    $operations = parent::getOperations($entity);
    +    if ($entity->access('update')) {
    +      $uri = $entity->uri();
    +      $operations['export'] = array(
    +        'title' => t('Export'),
    +        'href' => $uri['path'] . '/export',
    +        'options' => $uri['options'],
    +        'weight' => 30,
    +      );
    +    }
    +    return $operations;
    +  }
    

    just wondering, and this will be a follow up if anything, but do you think we should add a setting to entity info or just a controller property for whether the entity should provide an export form. Similar to what we do for status.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.27 KB
23.22 KB
FAILED: [[SimpleTest]]: [MySQL] 58,457 pass(es), 6 fail(s), and 2 exception(s). View

Rerolled, and addressed 24.1

Status: Needs review » Needs work

The last submitted patch, config-export-1898794-26.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.03 KB
24.19 KB
PASSED: [[SimpleTest]]: [MySQL] 58,224 pass(es). View

Whoops, bad reroll on my part. I dropped the one hunk and put the other in the wrong order.

beejeebus’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
git applyc https://drupal.org/files/config-entity-1898794-28.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 24767  100 24767    0     0  17629      0  0:00:01  0:00:01 --:--:-- 20267
error: patch failed: core/modules/views_ui/lib/Drupal/views_ui/ViewEditFormController.php:674
error: core/modules/views_ui/lib/Drupal/views_ui/ViewEditFormController.php: patch does not apply
tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
24.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,438 pass(es). View

Breaking my own patches now (that was just a $this->t() conflict)

webchick’s picture

Category: feature » task
Priority: Normal » Major

Since this is a regression, it seems more task-y than feature-y.

beejeebus’s picture

Status: Needs review » Reviewed & tested by the community
tim.plunkett’s picture

#31: config-entity-1898794-31.patch queued for re-testing.

tim.plunkett’s picture

Issue summary: View changes

added issue summary

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I'd like to see a bit of help text in https://drupal.org/files/export2.png. Without an explanation, this will be rather confusing to most users.

We should also fix the capitalization '(e.g. 'Export View" -> "Export view").

Otherwise looks good!

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.85 KB
22.68 KB
1.01 KB
24.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,900 pass(es). View

I'm honestly unsure of what to say here.
First of all, we don't yet have #1876906: Implement hook_help() for views_ui.module.

Secondly, a page like admin/config/development/export (the closest approximation to this) just says:

Use the export button below to download your site configuration.

I'd like to consider merging the help text into the views_ui_help() issue, and getting this in now.

However, someone on IRC suggested we include the filename, instead of the (useless) name of the entity type.

Here is a screenshot of this in D7:
Screen Shot 2013-09-06 at 11.50.17 AM.png

Here is the new screenshot for D8:
Screen Shot 2013-09-06 at 11.50.07 AM.png

jibran’s picture

Status: Needs review » Needs work

Some minor issues.

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityExportForm.php
    @@ -0,0 +1,82 @@
    +      $container->get('plugin.manager.entity'),
    

    entity.manager now.

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityExportForm.php
    @@ -0,0 +1,82 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function actions(array $form, array &$form_state) {
    +    // Export does not provide actions.
    +    return array();
    +  }
    

    I think we can add cancel link or back button. Just a suggestion.

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageControllerInterface.php
    @@ -0,0 +1,101 @@
    +   * Create configuration upon synchronizing configuration changes.
    

    Creates

  4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageControllerInterface.php
    @@ -0,0 +1,101 @@
    +   * Delete configuration upon synchronizing configuration changes.
    

    Deletes

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
24.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,853 pass(es). View

1) Fixed (there are others in core btw)
2) They have a browser back button or the breadcrumbs. We're fixing a regression, D7 didn't have a link.
3+4) That exists in HEAD, this is just moved, but fixed all the same.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

There is no action available on the page, so cancel might be even more confusing.

I think show the filename is enough for some context to figure out what is going on.

damiankloip’s picture

Agreed, +1. This gets us back to where we were.

jibran’s picture

Thank you for the changes @tim.plunkett. +1 for RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, but I am with Dries here. That's really awesome that I have an export of my View but... now what? Unlike Views in D7 and below, there's no "Import" to put it into, so this form is a bit of a dead end.

Looking at the patch, this doesn't seem like something we'd want to add in views_help() (or else every single module exposing an "export" key would have to do the same), but rather something we'd want embedded into the form itself.

So I'm thinking maybe a #description on $form['export'] something like:

"Copy and paste this code to your staging configuration directory." (link to wherever docs are that explain what the heck that is.)

moshe weitzman’s picture

Well, let's add a UI for pasting in the contents of a config file, then. It would be useful for add/edit operations. It would not be used for rename or delete. It would be a simple form which asks for a config name and config body. This would complement our existing form which is for importing a tarball of config files. Drush already does this - the code is pretty simple. I *might* even be able to code it.

In any case, I think we could proceed with this and let core or contrib add the "import single file" form.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
24.44 KB
PASSED: [[SimpleTest]]: [MySQL] 59,174 pass(es). View
769 bytes
36.3 KB

#42 fair enough. This links to https://drupal.org/documentation/administer/config

Screen Shot 2013-09-10 at 8.32.39 PM.png

#43 sounds like an interesting follow-up.

beejeebus’s picture

yeah, i like the fact that we are trying to raise the 'what do i do with this?' question, but moving this to the staging dir is not an option.

we don't import single files form there, so we need better help text than that. thinking on it.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

See, that's another reason I didn't want to add help text.
Because of the full-tree-config import thing, this is useless for site building.

It's main usecase was for support requests. The Views queue is driven by people uploading their view so we can spot problems.

Ideally a module developer wanting to provide their own config would be able to find it in their install...

If a single-file import was implemented, this would certainly be insanely useful, and we could document how to use them in conjunction.

So, we have a fixed regression, a patch with unhelpful/misleading UI text, and no suggestions yet. I'm unassigning myself.

dawehner’s picture

FileSize
23.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,948 pass(es). View
680 bytes

It's main usecase was for support requests. The Views queue is driven by people uploading their view so we can spot problems.

I can't tell you how important that was when doing support questions. People can't and also don't want really describe how they had setup their view.

If people click on that link they either know what they really need, or they just got asked to do so.

The title works with #title now, so let's remove the extra file.

dawehner’s picture

FileSize
44.22 KB

Hey, missed the screenshot.
export.png

beejeebus’s picture

Status: Needs review » Reviewed & tested by the community

i think this is good to go.

would be great to do what Moshe suggests in #43 in a follow up.

webchick’s picture

Argh. I'm still really torn here. I really do not intend to frustrate anyone, but please hear me out:

1) The feature that this is a regression from is actually two parts... the ability to export *and* the ability to import an export. This patch only covers one half. While it seems unfair to force both in the same patch, this feature is relatively useless on its own. It in fact leaves the user at a total dead-end, not at all sure what to do with all of the code they see. I worry they start doing something stupid with it, like over-writing their active config by hand, if we don't give them some guidance.

2) However, providing said guidance is tricky because there is no import, and so in fact the only use for this feature on its own, since we don't support partial config imports, is as a support tool, as Daniel says. However, we generally do not ship core with this kind of tool, leaving that up to the Devel module or similar.

3) However, if we were going to start shipping Drupal with a support tool like this, it makes zero sense to do it just for Views. It should rather be a "Configuration inspector" (or whatever) menu item off the Development menu, and it should work for any config entity.

So I'm honestly not sure what to do here. I can see three possible options:

1) Postpone this patch until a corresponding "import" feature is ready and combine the both of them into one patch.
2) Move this issue to the Devel module.
3) Move it to a centralized location for all config entities.

Not moving down from RTBC just yet, but would love your thoughts.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

mtift’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
23.47 KB
FAILED: [[SimpleTest]]: [MySQL] 58,833 pass(es), 2 fail(s), and 13 exception(s). View

Reroll

Status: Needs review » Needs work

The last submitted patch, config_entity-export-1898794-52.patch, failed testing.

heyrocker’s picture

Personally I would put this into Devel. I agree that without an import the feature doesn't make a ton of sense in core and could end up causing confusion. We aren't going to have a single-file import this release cycle, so that answer isn't really an option.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.95 KB
PASSED: [[SimpleTest]]: [MySQL] 59,193 pass(es). View
16.41 KB
PASSED: [[SimpleTest]]: [MySQL] 59,280 pass(es). View
23.46 KB
FAILED: [[SimpleTest]]: [MySQL] 59,227 pass(es), 2 fail(s), and 0 exception(s). View
655 bytes

Devel would be out backup option. But it might be nice to have the infrastructure in place first...

Here are three patches:

A) is a reroll of #52 after the path/pattern/route_name changes
B) is the code for this patch, but not used by Views (only by the test config entity)
C) is the addition of the interface but no actual changes, since that would be needed for a third-party improvement.

Interdiff is from 52 to A. B and C was just removing more and more code, no interdiff provided.

dawehner’s picture

Well, let's be honest providing support for views in core is pretty much hopeless anyway.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Issue tags: +Needs tests
FileSize
8.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,921 pass(es). View

Better update coming, just posting before I forget.

Status: Needs review » Needs work
Issue tags: -Needs tests, -DX (Developer Experience), -Configuration system, -VDC, -Configurables

The last submitted patch, config-1898794-57.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +DX (Developer Experience), +Configuration system, +VDC, +Configurables

#57: config-1898794-57.patch queued for re-testing.

tim.plunkett’s picture

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs review » Closed (duplicate)

Actually, this entire issue was about recreating the UI flow from D7, which is that you export your entity (view) from their own UI (views listing).

The new issue is a single centralized location for everything, and as such, I'm going to mark this a duplicate of the new approach.

It also handles non-config-entity CMI.

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.