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.


CommentFileSizeAuthor
#57 config-1898794-57.patch8.08 KBtim.plunkett
#55 interdiff.txt655 bytestim.plunkett
#55 config_entity-export-1898794-55-A.patch23.46 KBtim.plunkett
#55 config_entity-export-1898794-55-B.patch16.41 KBtim.plunkett
#55 config_entity-export-1898794-55-C.patch7.95 KBtim.plunkett
#52 config_entity-export-1898794-52.patch23.47 KBmtift
#48 export.png44.22 KBdawehner
#47 interdiff.txt680 bytesdawehner
#47 config_entity-export-1898794-47.patch23.47 KBdawehner
#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
#38 config-export-1898794-38.patch24.23 KBtim.plunkett
#38 interdiff.txt1.89 KBtim.plunkett
#36 config-entity-1898794-36.patch24.24 KBtim.plunkett
#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
#28 config-entity-1898794-28.patch24.19 KBtim.plunkett
#28 interdiff.txt2.03 KBtim.plunkett
#26 config-export-1898794-26.patch23.22 KBtim.plunkett
#26 interdiff.txt5.27 KBtim.plunkett
#23 config-entity-1898794-23.patch25.15 KBtim.plunkett
#21 config-entity-1898794-21.patch25.2 KBtim.plunkett
#21 interdiff.txt1.35 KBtim.plunkett
#19 config-export-1898794-19.patch25.2 KBtim.plunkett
#19 interdiff.txt2.91 KBtim.plunkett
#17 config-entity-1898794-17.patch24.86 KBtim.plunkett
#17 interdiff.txt2.59 KBtim.plunkett
#15 config-entity-1898794-15.patch23.7 KBtim.plunkett
#15 interdiff.txt6.47 KBtim.plunkett
#8 config-entity-1898794-8.patch17.91 KBtim.plunkett
#3 export.png11.22 KBdawehner
#3 export2.png36.34 KBdawehner
#3 drupal-1898794-3.patch4.55 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

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

Anonymous’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

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

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

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

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

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

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

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

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

Anonymous’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

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.

Anonymous’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

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

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

Anonymous’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

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

Anonymous’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

Reroll

Status: Needs review » Needs work

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

gdd’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

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

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.