Replace ctools_include('export') with Configurable Thingies

This implements the views entity type, that extends the new configuration entities. The views can at the moment just be loaded. This will definitely fail the bot atm.

Very much a work in progress.

Let's see whether the tests aren't broken.

Take that!

Recent changes should fixed a lot of the php errors.

+++ b/lib/Drupal/views/ViewDisplay.phpundefined
@@ -0,0 +1,47 @@
+  public function __construct(array $display_options = array()) {

+++ b/lib/Drupal/views/ViewStorage.phpundefined
@@ -2,177 +2,30 @@
+class ViewStorage extends ConfigurableBase {
...
+    parent::__construct($values, 'view');

+++ b/lib/Drupal/views/ViewStorageController.phpundefined
@@ -0,0 +1,139 @@
+class ViewStorageController extends ConfigStorageController {

+++ b/views.moduleundefined
@@ -1570,8 +1571,28 @@ function views_get_applicable_views($type) {
+function views_storage_save(View $view) {

Just a note: all these need docblocks. :)

Maybe this fixes some of the tests, you never know.

#13: 1741154-config-13.patch queued for re-testing.

Replaced the default views by default views in config, so this stuff should run now.

This patch should at least fix a lot of the exceptions.

A lot of the remaining exceptions are connected directly with the ctools export ui system.
Here are some more fixes.

This has the change to fix a lot of the errors.

This should fix the notices.

This could probably fix all of them.

-    $view = new View();
+    $view = new View(array(), 'view');

If we *always* need to call a view like this in code. I suggest making a custom constructor with no arguments, calling the parent with the needed arguments.

Would be a LOT cleaner :)

But this is sadly not possible as the constructor is part of the entity interface.

WHYYYYY do you want a constructor in an interface. I consider that a core bug... Seriously...

In the long run i think it probably makes sense to maybe
remove the display options.

On the viewExecutor $view->display['foo'] would contain directly the display handler.
On the storage view ->display[''] would contain just the config information which might or might not make things
easier to understand.

Made some minor docu changes and removed even more code.

.

Just some mostly nitpicky docs stuff. Is all this in @damiankloip's sandbox? If so I'll apply corrections there.

1. +++ b/lib/Drupal/views/Plugin/views/relationship/GroupwiseMax.phpundefined
   @@ -162,7 +162,7 @@ class GroupwiseMax extends RelationshipPluginBase {
   -    $view = new View();
   +    $view = new View(array(), 'view');
   

   So in this patch we're creating new view objects three ways: Constructing them with the namespace used, constructing them with the fully qualified classname, and using entity_create(). Let's add comments or a helper method for the tests or something. :)

2. +++ b/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
   @@ -0,0 +1,272 @@
   + * Tests that functionality of the the ViewStorageController.
   

   s/that/the/ and s/the the/the/

3. +++ b/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
   @@ -0,0 +1,272 @@
   +   * Tests adding/saving/loading displays on configurables.
   

   Probably "adding, saving, and loading".

4. +++ b/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
   @@ -0,0 +1,272 @@
   +    // Take sure the right display_plugin is created/instantiated.
   

   "Ensure" rather than "Take sure". :)

5. +++ b/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
   @@ -0,0 +1,272 @@
   +   * Load a single Configurable object from the controller.
   +   *
   +   * @param string $view_name
   +   *
   +   * @return object Drupal\views\View.
   

   The summary should start with "Loads", and the @param and @return need descriptions.

6. +++ b/lib/Drupal/views/ViewDisplay.phpundefined
   @@ -0,0 +1,47 @@
   + * A display type in a view.
   

   The class description should start with a verb.

7. +++ b/lib/Drupal/views/ViewDisplay.phpundefined
   @@ -0,0 +1,47 @@
   +  public function __construct(array $display_options = array()) {
   

   Need a docblock for the constructor.

8. +++ b/views.moduleundefined
   @@ -1569,8 +1570,53 @@ function views_get_applicable_views($type) {
   + * Load a view with the storage controller.
   ...
   + * Save a view with the storage controller.
   

   Should be "saves"/"loads" instead of "save"/"load".

9. +++ b/views.moduleundefined
   @@ -1569,8 +1570,53 @@ function views_get_applicable_views($type) {
   + * Change the status of a view.
   

   it's 100% clear what this does from reading the code, but the summary is more vague here. How about "Toggles a view's status between enabled and disabled"?

10. +++ b/views.moduleundefined
    @@ -1668,45 +1714,41 @@ function views_get_views_as_options($views_only = FALSE, $filter = 'all', $exclu
    + * @var Drupal\views\View
    ...
    + * @var Drupal\views\View
    

    These should be @param rather than @var.

11. +++ b/views.moduleundefined
    @@ -1668,45 +1714,41 @@ function views_get_views_as_options($views_only = FALSE, $filter = 'all', $exclu
    + *   The view object of which the status is checked.
    ...
    + *   The view object of which the status is checked.
    

    We can say just "The view object to check."

12. +++ b/views.moduleundefined
    @@ -1668,45 +1714,41 @@ function views_get_views_as_options($views_only = FALSE, $filter = 'all', $exclu
    + * Get a view from the config.
    

    How about "Loads a view from configuration"?

13. +++ b/views.moduleundefined
    @@ -1668,45 +1714,41 @@ function views_get_views_as_options($views_only = FALSE, $filter = 'all', $exclu
      * @param $name
      *   The name of the view.
    - * @param $reset
    - *   If TRUE, reset this entry in the load cache.
      * @return Drupal\views\View
    - *   A reference to the $view object. Use $reset if you're sure you want
    - *   a fresh one.
    + *   A reference to the $view object.
    

    We need a blank line between the @param and @return.

Here are the docs cleanups I just pushed to the branch. I think we should clean up the ViewDisplay constructor a bit. We should probably pass three parameters: the ID, the plugin ID/type/whatever, and then an array of additional options. That way we can fail meaningfully if required properties are missing, and provide sensible defaults for the less critical ones.

That way we also don't have to pack up so many arrays everywhere to construct these. ;)

Not sure if I can RTBC a patch I've written alot of.... but the tidy up looks great! Obviously on passing.

Lots of great code cleanup in there. Thanks @tim.plunkett!

1. +++ b/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
   @@ -35,6 +35,20 @@ class ViewStorageTest extends WebTestBase {
   +   * The Configurable information.
   

   Some more details here might be nice.

2. +++ b/lib/Drupal/views/ViewStorage.phpundefined
   @@ -43,17 +43,18 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
   +   *   (optional) The plugin type from the Views plugin data. Defaults to 'page'.
   
   @@ -156,20 +154,22 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
   +   *   (optional) The plugin type from the Views plugin data. Defaults to 'page'.
   

   81 chars. :P

3. +++ b/lib/Drupal/views/ViewStorage.phpundefined
   @@ -43,17 +43,18 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
   +   *   Either the key to the display in $view->display, so that the new display
   +   *   can be easily located, or FALSE if no $plugin_id was provided.
   

   This is a little awkward. How about simply:
   "The key to the display in $view->display, or FALSE if no plugin ID was provided.

4. +++ b/lib/Drupal/views/ViewStorage.phpundefined
   @@ -65,13 +66,12 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
   +      // Generate a unique human readable name. Therefore find out how often the
   +      // display_plugin already got used, which is stored at the end of the $id,
   +      // for example 'page_1'.
   
   @@ -81,13 +81,11 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
   +        // If we had more then one instance already attach the count, so you end
   +        // up with "Page" and "Page 2" for example.
   

   Let's anglify these comments a little. Also, "human-readable" should be hyphenated.

5. +++ b/lib/Drupal/views/ViewStorage.phpundefined
   @@ -107,10 +105,10 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
   +   * @param string $type
   +   *   Which plugin should be used for the new display ID.
   ...
      function generate_display_id($type) {
   

   Did we change $type to $plugin_id elsewhere? In either case, let's add an example. (E.g. 'page', right?)

6. +++ b/lib/Drupal/views/ViewStorage.phpundefined
   @@ -156,20 +154,22 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
   +   * @param string $id
   +   *   (optional) The ID to use. Defaults to NULL.
   

   Since we asked about this in person, let's add examples wherever this parameter appears? (page_1, page_2, etc., right?)

   Edit: and/or an @see to the method where it's already more thoroughly documented elsewhere in the class.

7. +++ b/lib/Drupal/views/ViewStorage.phpundefined
   @@ -156,20 +154,22 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
   +  function &new_display($plugin_id = 'page', $title = NULL, $id = NULL) {
   +    $id = $this->add_display($plugin_id, $title, $id);
   

   Whoa. Note to myself to ask about this.

8. +++ b/lib/Drupal/views/ViewStorage.phpundefined
   @@ -190,9 +190,26 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
   -   * Add an item with a handler to the view.
   +   * Adds an item with a handler to the view.
   
   @@ -241,8 +262,17 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
   -   * Get the configuration of an item (field/sort/filter/etc) on a given
   -   * display.
   +   * Gets the configuration of an item on a given display.
   
   @@ -282,11 +318,21 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
   +   * Sets an option on an item.
   

   Let's clarify more what "items" are in the documentation for all these methods. Gets the configuration of an abstract noun!
   item == item with a handler == anything that has a handler?

9. +++ b/lib/Drupal/views/ViewStorage.phpundefined
   @@ -204,29 +221,33 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
   +    // If the desired type is not known, use it directly.
   +    $handler_type = !empty($types[$type]['type']) ? $types[$type]['type'] : $type;
   +
   +    $handler = views_get_handler($table, $field, $handler_type);
   +
   +    $fields[$id] = array(
   

   I like the code cleanup here, but I don't understand the second half of the comment.

10. +++ b/lib/Drupal/views/ViewStorage.phpundefined
    @@ -204,29 +221,33 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
    +   *   (optional) A specific display machine name to use, otherwise the current
    +   *   display will be used.
    

    Otherwise (and the comma) are not grammatical here. Let's say something like:
    A specific display machine name to use. If NULL, the current display will be used.

11. +++ b/lib/Drupal/views/ViewStorage.phpundefined
    @@ -257,18 +287,24 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
    +   * Sets the configuration of an item on a given display.
    

    Do the setters want to return something? (The answer might be no. I need to check the full patch.)

12. +++ b/lib/Drupal/views/ViewStorage.phpundefined
    @@ -282,11 +318,21 @@ class ViewStorage extends ConfigurableBase implements ViewStorageInterface {
    +   * Use this only if you have just 1 or 2 options to set; if you have many,
    +   * consider getting the item, adding the options and doing set_item directly.
    

    set_item() should have parens. Also let's add an @see to the other method at the bottom of the docblock, and from the other method's docblock to this method.

Whoops, xposted.

Note that I haven't reviewed the final diff between the sandbox and 8.x-3.x yet; I just reviewed Tim's interdiff.