Part of meta #1853522: [META] (Re)introduce Views data integration for core modules. From the 8.x-3.x branch of Views.

How to test(from #66):

  1. Added two different books, one with child pages.
  2. Added a view that pulled all book content.
  3. Added a relationship to top-level book.
  4. Filtered by the id of the top-level book.
  5. The view properly showed all the pages of book one and excluded book two.
CommentFileSizeAuthor
#91 90-inter.diff2.78 KBfrob
#91 reintroduce_views-1853524-90.patch40.38 KBfrob
#89 reintroduce_views-1853524-89.patch42.34 KBfrob
#85 reintroduce_views-1853524-85.patch42.32 KBfrob
#81 interdiff-1853524-71-81.txt2.76 KBfrob
#81 reintroduce_views-1853524-81.patch45.45 KBfrob
#77 interdiff-62.patch8.01 KBfrob
#77 interdiff.patch1018 bytesfrob
#77 1853524-reintroduce-view-book-integration-77.patch45.37 KBfrob
#67 1853524-diff-62-67.txt6.78 KBvijaycs85
#67 1853524-67.patch44.95 KBvijaycs85
#62 1853524-62.patch42.23 KBfrob
#54 1853524-54.patch42.23 KBolli
#49 interdiff.txt2.25 KBolli
#49 1853524-49.patch44.97 KBolli
#45 interdiff.txt31.1 KBolli
#45 1853524-45.patch44.38 KBolli
#37 drupal8.book-module.1853524-37.patch38.26 KBjibran
#37 interdiff.txt2.67 KBjibran
#33 drupal8.book-module.1853524-33.patch39.48 KBjibran
#33 interdiff.txt33.4 KBjibran
#33 interdiff.views_.data_.txt1.21 KBjibran
#32 drupal8.book-module.1853524-32.patch5.29 KBjibran
#32 interdiff.txt5.92 KBjibran
#30 drupal8.book-module.1853524-30.patch5.33 KBjibran
#30 interdiff.txt4.3 KBjibran
#28 drupal8.book-module.1853524-28.patch5 KBjibran
#28 interdiff.txt2.62 KBjibran
#22 Book-view-2013-10-15-13.26.10.png87.47 KBvijaycs85
#21 drupal8.book-module.1853524-21.patch5.08 KBjibran
#21 interdiff.txt3.06 KBjibran
#14 1853524-14.patch5.14 KBeriknewby
#14 interdiff.txt939 byteseriknewby
#11 1853524.patch5.11 KBeriknewby
#11 interdiff.txt548 byteseriknewby
#10 1853524.patch5.11 KBeriknewby
#10 interdiff.txt547 byteseriknewby
#5 1853524-5.patch5.11 KBjibran
#5 interdiff.txt3.84 KBjibran
#3 1853524-3.patch4.05 KBjibran
#3 interdiff.txt946 bytesjibran
book.patch4.05 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, book.patch, failed testing.

Bojhan’s picture

How much of this is integration, seems bid etc are still missing? I dont think those are feature requests.

jibran’s picture

Status: Needs work » Needs review
FileSize
946 bytes
4.05 KB

reroll.

dawehner’s picture

+++ b/core/modules/book/book.views.incundefined
@@ -0,0 +1,113 @@
+ * Provide views data and handlers for book.module.

I think it should be just "Provide views data for book.module."

+++ b/core/modules/book/book.views.incundefined
@@ -0,0 +1,113 @@
+  // book table
...
+    // There is no argument here; if you need an argument, add the relationship
+    // and use the node: nid argument.
...
+  // Book hierarchy and weight data are now in {menu_links}.
...
+  // The {book} record for the parent node.

I think these comments don't give you any information, but the other ones do.

+++ b/core/modules/book/lib/Drupal/book/Plugin/views/argument_default/Root.phpundefined
@@ -0,0 +1,36 @@
+ * Definition of Drupal\book\Plugin\views\argument_default\Root.

... Should be contains.

+++ b/core/modules/book/lib/Drupal/book/Plugin/views/argument_default/Root.phpundefined
@@ -0,0 +1,36 @@
+  function getArgument() {

Needs inheritdoc

+++ b/core/modules/book/lib/Drupal/book/Plugin/views/argument_default/Root.phpundefined
@@ -0,0 +1,36 @@
+      $node = node_load($nid);

You could inject the entity storage controller in order to load the node.

jibran’s picture

FileSize
3.84 KB
5.11 KB

Fixed #4. Do we need tests?

dawehner’s picture

Issue tags: +VDC, +Need tests

I fear we should have some.

jibran’s picture

Status: Needs review » Needs work
+++ b/core/modules/book/lib/Drupal/book/Plugin/views/argument_default/Root.phpundefined
@@ -0,0 +1,72 @@
+   * The entity manager service

The node storage controller.

dawehner’s picture

Issue tags: +Novice

That is easy to fix.

eriknewby’s picture

Assigned: Unassigned » eriknewby

Assigned via drupalmentoring.org - update following shortly.

eriknewby’s picture

Assigned: eriknewby » Unassigned
Status: Needs work » Needs review
FileSize
547 bytes
5.11 KB

added changes from #7 - updated docblock.

eriknewby’s picture

FileSize
548 bytes
5.11 KB

Forgot a period.

xjm’s picture

Issue tags: -Novice

Thanks @eriknewby!

So next we just need those tests.

dawehner’s picture

Issue tags: +Novice
+++ b/core/modules/book/lib/Drupal/book/Plugin/views/argument_default/Root.phpundefined
@@ -0,0 +1,72 @@
+    return new static($configuration, $plugin_id, $plugin_definition, $container->get('plugin.manager.entity')->getStorageController('node'));

Let's put everything in a new line, so it is easier to adapt later.

eriknewby’s picture

FileSize
939 bytes
5.14 KB

Hopefully I understood you correctly. Please see new patch attached.

dawehner’s picture

Perfect.

oadaeh’s picture

Issue tags: -Need tests +Needs tests

Changing the tests tag to the more universally used one.

jibran’s picture

Nitesh Sethia’s picture

Assigned: Unassigned » Nitesh Sethia

This patch is working fine and there is no need for any rerolling.

jibran’s picture

Status: Needs review » Needs work

Hi @Nitesh Sethia. Yes patch is working fine but views api is improved in #1969388: Change notice: Add dedicated annotations for Views plugins and old api is still working as well but for all new patches we have to use new api so that core can remove old api without any problem when needed. I hope this will address your concern.

dawehner’s picture

+++ b/core/modules/book/lib/Drupal/book/Plugin/views/argument_default/Root.php
@@ -0,0 +1,77 @@
+      $nodes = $this->nodeStorage->load(array($nid));
+      $node = reset($nodes);

This also has to be adapted to load just a single node.

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.06 KB
5.08 KB

Fixed #7 #13, #17 and #20. inter diff is against #5.

vijaycs85’s picture

Tested this manually and here is some finding:

1. Patch in #21 applies with current code base.
2. Found a regression #2112237: Regression: Remove EntityFormControllerNG from book module - Filed and issued patch (Not related to this issue).
3. Can see the book plugin for views (ref: Book-view-2013-10-15-13.26.10.png)

Overall, This looks good to me. +1 to RTBC.

vijaycs85’s picture

Issue tags: -Needs tests, -Novice, -VDC
xjm’s picture

Issue summary: View changes
Issue tags: +VDC, +beta target, +Needs tests, +Novice
xjm’s picture

Status: Needs review » Needs work

This needs test coverage.

xjm’s picture

Priority: Normal » Major
martin107’s picture

jibran’s picture

Assigned: Nitesh Sethia » jibran
Status: Needs work » Needs review
FileSize
2.62 KB
5 KB

First of all reroll. I am going to try to write some tests.

PS: @dawehner thinks I am novice so he pinged me about this issue :D j/k

jibran’s picture

Status: Needs review » Needs work
Issue tags: -Novice

I have to re-write the book_views_data after #2084421: Phase 2 - Decouple book module schema from menu links

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Needs review
FileSize
4.3 KB
5.33 KB

Here is the new patch with updated book_views_data. @dawehner can you please confirm that current changes are correct? And can you please elaborate what kind of test view we are talking about here?
Do we want to replace 'Book navigation' block with view?

dawehner’s picture

The code looks really solid.

Something like the following would be great for the test. Setup a bunch of book nodes, with some hierarchy. Create a view to show this books, together with an argument using this new argument default plugin.
Ensure, that you can list and filter book entries somehow how you expect it.

  1. +++ b/core/modules/book/book.views.inc
    @@ -0,0 +1,117 @@
    +  $prefix = ['th', 'st', 'nd', 'rd', 'th', 'th', 'th', 'th', 'th', 'th'];
    ...
    +      'title' => t("$i{$prefix[$i]} Parent"),
    +      'help' => t("The $i{$prefix[$i]} parent of book node."),
    

    Did we considered to use a placeholder in the string instead?

  2. +++ b/core/modules/book/src/Plugin/views/argument_default/Root.php
    @@ -0,0 +1,77 @@
    +class Root extends Node implements ContainerFactoryPluginInterface {
    ...
    +   * Constructs a Drupal\Component\Plugin\PluginBase object.
    

    This should point to the same class...

  3. +++ b/core/modules/book/src/Plugin/views/argument_default/Root.php
    @@ -0,0 +1,77 @@
    +   * @var \Drupal\Core\Entity\EntityStorageInterface
    ...
    +    $this->nodeStorage = $node_storage;
    ...
    +      $container->get('entity.manager')->getStorage('node')
    

    Did we considered to point to the dedicated NodeStorageInterface?

jibran’s picture

Assigned: Unassigned » jibran
FileSize
5.92 KB
5.29 KB

Thanks for the review and these are all very good points. I have fixed all of them. I am working on a test view.

jibran’s picture

Assigned: jibran » Unassigned
Issue tags: -Needs tests +Pre-AMS beta sprint
FileSize
1.21 KB
33.4 KB
39.48 KB

Thanks @dawehner for the pointers and @ClemensTolboom for all the support.

clemens.tolboom’s picture

  1. +++ b/core/modules/book/book.views.inc
    @@ -0,0 +1,127 @@
    +  $parents = [
    +    1 => t('1st parent'),
    +    2 => t('2nd parent'),
    +    3 => t('3rd parent'),
    +    4 => t('4th parent'),
    +    5 => t('5th parent'),
    +    6 => t('6th parent'),
    +    7 => t('7th parent'),
    +    8 => t('8th parent'),
    +    9 => t('9th parent'),
    +  ];
    

    Ignore when this issue is just about to fix the old way. Otherwise these terms are confusing. I expected the node parent to be its '1st parent' aka up()

  2. +++ b/core/modules/book/src/Plugin/views/argument_default/Root.php
    @@ -0,0 +1,75 @@
    +class Root extends Node implements ContainerFactoryPluginInterface {
    

    Don't call this Root (its too short). Maybe TopLevelNode or BookRoot or RootNode

  3. Adding the depth argument and set it to fixed value ie 1,2,3 it works.

    But adding another parameter to the viewpreview like book-nav/?x=1 does not show any items. I guess that has nothing to do with this patch.

Status: Needs review » Needs work

The last submitted patch, 33: drupal8.book-module.1853524-33.patch, failed testing.

damiankloip’s picture

  1. +++ b/core/modules/book/book.views.inc
    @@ -0,0 +1,127 @@
    +      'click sortable' => TRUE,
    

    This is TRUE by default.

  2. +++ b/core/modules/book/book.views.inc
    @@ -0,0 +1,127 @@
    +  foreach ($parents as $i => $parent) {
    

    Just call this $label or something.

jibran’s picture

Status: Needs work » Needs review
FileSize
2.67 KB
38.26 KB

Fixed #34.2 and #36

+++ b/core/modules/views/src/EntityViewsData.php
@@ -153,7 +153,7 @@ public function getViewsData() {
+        'type' => 'LEFT'

@@ -169,7 +169,7 @@ public function getViewsData() {
+        'type' => 'LEFT',

@@ -179,7 +179,7 @@ public function getViewsData() {
+          'type' => 'LEFT',

I have no idea how to fix that. I tried creating relationship plugin but nothing worked. Need some help here.

Status: Needs review » Needs work

The last submitted patch, 37: drupal8.book-module.1853524-37.patch, failed testing.

dawehner’s picture

Meh.

jibran’s picture

Meh.

What?

dawehner’s picture

Its sad that this issue wasn't been able to get in, no offence.

jibran’s picture

Do we have a solution for this yet?

jibran’s picture

I think we can re-roll this patch now.

vijaycs85’s picture

The patch applies without any conflict. We might need to address test fails and #37

olli’s picture

Status: Needs work » Needs review
FileSize
44.38 KB
31.1 KB

I could't make the last patch work anymore so I changed book_views_data() so that it joins to 'node_field_data' rather than 'node'. Tried to recreate the test view from scratch.

Here's some work with 28 exceptions (instead of 28 fails:).

olli’s picture

  1. +++ b/core/modules/book/config/schema/book.views.schema.yml
    @@ -0,0 +1,8 @@
    +views.argument_default.book_root:
    +  type: sequence
    +  label: 'Book root from current node'
    +  sequence:
    +    type: string
    +    label: 'Nid'
    

    I tried to use something like this

    views.argument_default.book_root:
      type: views.argument_default.node
      label: 'Book root from current node'
    

    , but that didn't work.

  2. +++ b/core/modules/views/src/Entity/Render/EntityFieldRenderer.php
    @@ -199,8 +199,9 @@ protected function buildFields(array $values) {
    -        $entity = $field->getEntity($result_row);
    -        $entities_by_bundles[$entity->bundle()][$result_row->index] = $this->getEntityTranslation($entity, $result_row);
    +        if ($entity = $field->getEntity($result_row)) {
    +          $entities_by_bundles[$entity->bundle()][$result_row->index] = $this->getEntityTranslation($entity, $result_row);
    +        }
    
    +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -394,7 +394,7 @@ public function getEntity(ResultRow $values) {
    -    else {
    +    elseif (isset($values->_relationship_entities[$relationship_id])) {
           return $values->_relationship_entities[$relationship_id];
         }
    

    This makes sense to me because the relationship is not required but it feels wrong to change this here.

jibran’s picture

WoW @olli impressive work. If I'd find you on IRC i'd give you 1000(oili+=1000) karma points for this.

Status: Needs review » Needs work

The last submitted patch, 45: 1853524-45.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
FileSize
44.97 KB
2.25 KB

Thanks @jibran +1000 to you too.

Here's a patch with an isset around that exception and an interdiff with more context.

jibran’s picture

I heard @larowlan saying a lot of times. It is essential for Drupal core that book and forum module works completely with their full functionality because it let us check a lot of modules i.e. node, taxonomy, comment and menu(book is decoupled form it but before) working together.
We can also see in this patch that there are a lot of minor things which needs better boundary checks. I'm overall plus plus on all the changes we have done in FieldPluginBase and EntityFieldRenderer. I'd like to RTBC it but I worked a lot on this issue so let's wait for @dawehner or @damiankloip to RTBC it.

  1. +++ b/core/modules/node/src/Plugin/views/argument_default/Node.php
    @@ -41,7 +41,6 @@ class Node extends ArgumentDefaultPluginBase implements CacheablePluginInterface
    -   *
    

    Unrelated.

  2. +++ b/core/modules/views/src/Entity/Render/EntityFieldRenderer.php
    @@ -143,7 +143,13 @@ public function render(ResultRow $row, Field $field = NULL) {
    +        $build_fields = $this->buildFields([$row]);
    +        if (isset($build_fields[$row->index])) {
    +          $build = $build_fields[$row->index][$field_id];
    +        }
    +        else {
    +          $build = [];
    +        }
    

    This is very good fix imo. I don't think we need tests for this. What do you think?

  3. +++ b/core/modules/views/src/Entity/Render/EntityFieldRenderer.php
    @@ -199,8 +205,9 @@ protected function buildFields(array $values) {
    +        if ($entity = $field->getEntity($result_row)) {
    

    Seems legit as well. Again, do we need test coverage for this?

  4. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -394,7 +394,7 @@ public function getEntity(ResultRow $values) {
    +    elseif (isset($values->_relationship_entities[$relationship_id])) {
    

    And again do we need test coverage for this one as well?

dawehner’s picture

It is so good to so some progress here!

  1. +++ b/core/modules/book/book.views.inc
    @@ -0,0 +1,125 @@
    +      'help' => t('The !parent of book node.', ['!parent' => $parent_label]),
    ...
    +        'label' => t('Book !parent', ['!parent' => $parent_label]),
    

    Do we really need an ! placeholder there?

  2. +++ b/core/modules/book/src/Tests/Views/BookRelationshipTest.php
    @@ -0,0 +1,202 @@
    +   */
    +  public function testRelationship() {
    +
    

    Did you considered to write that test with two nested for looops like (danger, pseudo code!!) :

    for ($i = 0; $i < 8; $i++) {$this->drupalGet('test-book/$nodes[$i]
    for ($j = 0; $j < $j; $j}}  {$this->assertLink($nodes[$j]...
  3. +++ b/core/modules/views/src/Entity/Render/EntityFieldRenderer.php
    @@ -143,7 +143,13 @@ public function render(ResultRow $row, Field $field = NULL) {
    +        if (isset($build_fields[$row->index])) {
    +          $build = $build_fields[$row->index][$field_id];
    +        }
    +        else {
    +          $build = [];
    +        }
    
    @@ -199,8 +205,9 @@ protected function buildFields(array $values) {
    +        if ($entity = $field->getEntity($result_row)) {
    +          $entities_by_bundles[$entity->bundle()][$result_row->index] = $this->getEntityTranslation($entity, $result_row);
    +        }
    

    Do you mind describing in one of the two places how it can happen that $build_fields is empty for that particular $row index? I totally believe that this is the case, I guess we are dealing with optional relationships, right? In case, do we need dedicated test coverage for it?

olli’s picture

Filed #2534780: Fatal error rendering fields using an optional relationship to fix that.

  1. +++ b/core/modules/book/config/schema/book.views.schema.yml
    @@ -0,0 +1,8 @@
    +views.argument_default.book_root:
    +  type: sequence
    +  label: 'Book root from current node'
    +  sequence:
    +    type: string
    +    label: 'Nid'
    

    Any ideas what to do with this schema? I just copied this from node and I don't think either of those have any options to configure?

  2. +++ b/core/modules/book/src/Plugin/views/argument_default/BookRoot.php
    @@ -0,0 +1,78 @@
    +class BookRoot extends Node {
    

    Should this implement some cache related interface and can we use 'route.book_navigation' cache context?

dawehner’s picture

Status: Needs review » Needs work

Any ideas what to do with this schema? I just copied this from node and I don't think either of those have any options to configure?

Mh, true. This doesn't really make sense.

Should this implement some cache related interface and can we use 'route.book_navigation' cache context?

Oh yeah good point. That context is basically what the view will vary by!

olli’s picture

jhedstrom queued 54: 1853524-54.patch for re-testing.

jhedstrom’s picture

+++ b/core/modules/book/src/Tests/Views/BookRelationshipTest.php
@@ -0,0 +1,202 @@
+  /**
+   * Creates a book node.
+   *
+   * @param int|string $book_nid
+   *   A book node ID or set to 'new' to create a new book.
+   * @param int|null $parent
+   *   (optional) Parent book reference ID. Defaults to NULL.
+   *
+   * @return \Drupal\node\NodeInterface
+   *   The book node.
+   */
+  protected function createBookNode($book_nid, $parent = NULL) {
+    // $number does not use drupal_static as it should not be reset
+    // since it uniquely identifies each call to createBookNode().
+    // Used to ensure that when sorted nodes stay in same order.
+    static $number = 0;
+
+    $edit = array();
+    $edit['title[0][value]'] = $number . ' - SimpleTest test node ' . $this->randomMachineName(10);
+    $edit['body[0][value]'] = 'SimpleTest test body ' . $this->randomMachineName(32) . ' ' . $this->randomMachineName(32);
+    $edit['book[bid]'] = $book_nid;
+
+    if ($parent !== NULL) {
+      $this->drupalPostForm('node/add/book', $edit, t('Change book (update list of parents)'));
+
+      $edit['book[pid]'] = $parent;
+      $this->drupalPostForm(NULL, $edit, t('Save'));
+      // Make sure the parent was flagged as having children.
+      $parent_node = \Drupal::entityManager()->getStorage('node')->loadUnchanged($parent);
+      $this->assertFalse(empty($parent_node->book['has_children']), 'Parent node is marked as having children');
+    }
+    else {
+      $this->drupalPostForm('node/add/book', $edit, t('Save'));
+    }
+
+    // Check to make sure the book node was created.
+    $node = $this->drupalGetNodeByTitle($edit['title[0][value]']);
+    $this->assertNotNull(($node === FALSE ? NULL : $node), 'Book node found in database.');
+    $number++;
+
+    return $node;
+  }

Since this also exists in BookTest, we could move this to a trait, instead of just copying the code over (this could also be a follow-up, but test-only patches are quite hard to get reviewed).

Aside from that nit, this is looking quite good assuming the tests still pass.

Lucasljj queued 54: 1853524-54.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 54: 1853524-54.patch, failed testing.

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev

This will need to be against 8.1.x now.

frob’s picture

I have queued for retesting. The patch (#54 applied cleanly and my manual test works. Lets see what the tests say. Is there anything else that needs to be done here?

The last submitted patch, 54: 1853524-54.patch, failed testing.

frob’s picture

I fixed the broken !parent to be @parent. This should allow the tests to pass.

frob’s picture

Status: Needs work » Needs review

Agh, crying children distracting me. Marking needs review.

raj45’s picture

It would be awesome to get this included in 8.1.x if possible, thanks to everybody for you work on Views integration for Book! Perhaps somebody more code-knowledgeable than me can review the code?

frob’s picture

While this is for inclusion into 8.1 this patch still applies cleanly to 8.0

In case anyone venturing here from google finds this patch.

illeace’s picture

Status: Needs review » Reviewed & tested by the community

To test this I:

  • Added two different books, one with child pages.
  • Added a view that pulled all book content.
  • Added a relationship to top-level book.
  • Filtered by the id of the top-level book.
  • The view properly showed all the pages of book one and excluded book two.
vijaycs85’s picture

Addressing #56. Though it's a minor, traits reduce the duplicate method.

Status: Needs review » Needs work

The last submitted patch, 67: 1853524-67.patch, failed testing.

frob’s picture

@vijaycs85, I would suggest opening a new ticket to address this in order to not block this working patch from being put into core. This issue was stagnant enough for us to just use something that works and isn't bad.

We had an RTBCed patch from #62 and we want this functionality to be included in 8.1. Unless you can provide a patch that can pass tests I think further development should be done in a new issue.

frob’s picture

Status: Needs work » Reviewed & tested by the community

I am marking this as RTBC with the working patch from #62 so we can get this functionality back with 8.1.

We can improve the implementation later in another issue.

vijaycs85’s picture

@frob: sorry for the delay. I have been working on the fix for test fail. Just made it green in my test issue #1970534-71: Patch testing issue. I can update here or can wait for a followup.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kattekrab’s picture

A shame, it looks like this just missed out on 8.1. Would it be disruptive?

frob’s picture

I don't think this would be a disruptive fix. No api changes, it is just re-introducing the view integration that was removed when views was included in core.

Without this patch the book module is very limited.

alexpott’s picture

Version: 8.2.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Needs work
  1. I think nice nice API addition is valid for 8.1.x-beta1 - but I think it'll need to be finished before 8.1.x-beta2
  2. FILE: ...rupal/core/modules/book/src/Tests/Views/BookRelationshipTest.php
    ----------------------------------------------------------------------
      17 | ERROR | [x] Separate the @group and @see sections by a blank
         |       |     line.
      47 | ERROR | [ ] Class property $book_author should use lowerCamel
         |       |     naming without underscores
     154 | ERROR | [ ] Inline doc block comments are not allowed; use "/*
         |       |     Comment */" or "// Comment" instead
    ----------------------------------------------------------------------
    PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
    ----------------------------------------------------------------------
    

    Above are a few coding standards fixes to fix.

  3. +++ b/core/modules/book/book.views.inc
    @@ -0,0 +1,125 @@
    +    'title' => t('Top level book'),
    +    'help' => t('The book the node is in.'),
    
    +++ b/core/modules/book/config/schema/book.views.schema.yml
    @@ -0,0 +1,8 @@
    +  label: 'Book root from current node'
    
    +++ b/core/modules/book/src/Plugin/views/argument_default/BookRoot.php
    @@ -0,0 +1,78 @@
    + *   id = "book_root",
    + *   title = @Translation("Book root from current node")
    ...
    +class BookRoot extends Node {
    

    I don't see why we're introducing a new concept of "book root" here - elsewere in the UI and code it is referred to as top level and I think we should use that language here.

  4. Nice to see relationship test coverage!
  5. #71 can be do in the patch or as a followup - not sure it matters
martin107’s picture

Just noticed a little thing, while following along

We are using the 'entity.manager' service which is deprecated, a small change - we should use 'entity_type.manager' for better long term support.

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static(
      $configuration,
      $plugin_id,
      $plugin_definition,
      $container->get('current_route_match'),
      $container->get('entity.manager')->getStorage('node')
    );
  }
frob’s picture

This is based off the patch from the link in #71. I am sure there is more that needs to be done. I just edited the patch.

I included an interdiff from 71 and from 62.

alexpott’s picture

Status: Needs work » Needs review

The last submitted patch, 77: 1853524-reintroduce-view-book-integration-77.patch, failed testing.

frob’s picture

Assigned: Unassigned » frob

Okay, so that didn't work. I was attempting to edit the patch directly. I am re-rolling the patch from 71 for real this time.

frob’s picture

Here is the latest roll, although I am not sure exactly what to do about the comment:

    // Add page hierarchy to book.
    // Book
    //  |- Node 0
    //   |- Node 1
    //    |- Node 2
    //     |- Node 3
    //      |- Node 4
    //       |- Node 5
    //        |- Node 6
    //         |- Node 7.

This will never pass coding standards. However, there are lots of coding standards issues in the book module currently.

Status: Needs review » Needs work

The last submitted patch, 81: reintroduce_views-1853524-81.patch, failed testing.

frob’s picture

Assigned: Unassigned » frob

Okay, I am not quite sure why that didn't pass, but I am re-rolling off of #62 this time. #71 can wait for a follow up issue.

xjm’s picture

This issue was marked as a beta target for the 8.0.x beta, but I am keeping it tagged as a beta target for 8.1.x as well per #75. (@alexpott forgot to mention in his comment that he discussed the issue with other committers as well.)

frob’s picture

I have rerolled the patch from 62 to include the stuff from 75.

frob’s picture

Status: Needs work » Needs review
frob’s picture

Assigned: frob » Unassigned
alexpott’s picture

+++ b/core/modules/book/src/Tests/Views/BookRelationshipTest.php
@@ -0,0 +1,210 @@
+    $this->book = $this->createBookNode('new');
+    $book = $this->book;
...
+    // Create new book.
+    // @var \Drupal\node\NodeInterface[] $nodes
+    $nodes = $this->createBook();
+    $this->drupalGet('test-book/' . $nodes[0]->id());
+    $this->assertLink($nodes[0]->label());
+    $this->drupalGet('test-book/' . $nodes[1]->id());

We never actually test the book. We also don't test what happens when a node id that is not a book is an argument.

frob’s picture

We never actually test the book.

I am not sure what you are asking for here. This book is just used to create the hierarchy. It is used over and over again in the createBookNode method.

We also don't test what happens when a node id that is not a book is an argument.

I am not sure we need to test that. I could be wrong, (I am just reading the patch, I didn't write the patch I am just re-rolling it over and over because I am using it) The nid is coming from the book table as joined into the view here.

+  $data['book']['table']['join'] = [
+    'node_field_data' => [
+      'left_field' => 'nid',
+      'field' => 'nid',
+    ],
+  ];

We are joining in the book table by the nid so there shouldn't be any non book nids used as arguments. Or am I simplifying this too much.

I did notice while reading the patch that I left a reference to the book root in there. That is fixed now.

dawehner’s picture

Status: Needs review » Needs work

This patch looks pretty ready!

  1. +++ b/core/modules/book/src/Plugin/views/argument_default/TopLevelBook.php
    @@ -0,0 +1,78 @@
    +/**
    + * @file
    + * Contains Drupal\book\Plugin\views\argument_default\TopLevelBook.
    + */
    +
    
    +++ b/core/modules/book/src/Tests/Views/BookRelationshipTest.php
    @@ -0,0 +1,210 @@
    +/**
    + * @file
    + * Contains \Drupal\book\Tests\Views\BookRelationshipTest.
    + */
    +
    

    @file should be dropped now.

  2. +++ b/core/modules/book/src/Tests/Views/BookRelationshipTest.php
    @@ -0,0 +1,210 @@
    +
    +    $this->book = $this->createBookNode('new');
    ...
    +
    +    $this->drupalLogout();
    +
    ...
    +  protected function createBookNode($book_nid, $parent = NULL) {
    +    // $number does not use drupal_static as it should not be reset
    +    // since it uniquely identifies each call to createBookNode().
    +    // Used to ensure that when sorted nodes stay in same order.
    +    static $number = 0;
    +
    +    $edit = array();
    +    $edit['title[0][value]'] = $number . ' - SimpleTest test node ' . $this->randomMachineName(10);
    +    $edit['body[0][value]'] = 'SimpleTest test body ' . $this->randomMachineName(32) . ' ' . $this->randomMachineName(32);
    +    $edit['book[bid]'] = $book_nid;
    +
    +    if ($parent !== NULL) {
    +      $this->drupalPostForm('node/add/book', $edit, t('Change book (update list of parents)'));
    +
    +      $edit['book[pid]'] = $parent;
    +      $this->drupalPostForm(NULL, $edit, t('Save'));
    +      // Make sure the parent was flagged as having children.
    +      $parent_node = \Drupal::entityManager()->getStorage('node')->loadUnchanged($parent);
    +      $this->assertFalse(empty($parent_node->book['has_children']), 'Parent node is marked as having children');
    +    }
    +    else {
    +      $this->drupalPostForm('node/add/book', $edit, t('Save'));
    +    }
    +
    +    // Check to make sure the book node was created.
    +    $node = $this->drupalGetNodeByTitle($edit['title[0][value]']);
    +    $this->assertNotNull(($node === FALSE ? NULL : $node), 'Book node found in database.');
    +    $number++;
    +
    +    return $node;
    +  }
    

    I'm wondering whether we better create the books via an api. Just a note: This test is about the views integration, not the other part of the book UI.

  3. +++ b/core/modules/book/src/Tests/Views/BookRelationshipTest.php
    @@ -0,0 +1,210 @@
    +    $this->drupalGet('test-book/' . $nodes[0]->id());
    +    $this->assertLink($nodes[0]->label());
    +    $this->drupalGet('test-book/' . $nodes[1]->id());
    +    $this->assertLink($nodes[0]->label());
    +    $this->assertLink($nodes[1]->label());
    +    $this->drupalGet('test-book/' . $nodes[2]->id());
    +    $this->assertLink($nodes[0]->label());
    +    $this->assertLink($nodes[1]->label());
    +    $this->assertLink($nodes[2]->label());
    +    $this->drupalGet('test-book/' . $nodes[3]->id());
    +    $this->assertLink($nodes[0]->label());
    +    $this->assertLink($nodes[1]->label());
    +    $this->assertLink($nodes[2]->label());
    +    $this->assertLink($nodes[3]->label());
    +    $this->drupalGet('test-book/' . $nodes[4]->id());
    +    $this->assertLink($nodes[0]->label());
    +    $this->assertLink($nodes[1]->label());
    +    $this->assertLink($nodes[2]->label());
    +    $this->assertLink($nodes[3]->label());
    +    $this->assertLink($nodes[4]->label());
    +    $this->drupalGet('test-book/' . $nodes[5]->id());
    +    $this->assertLink($nodes[0]->label());
    +    $this->assertLink($nodes[1]->label());
    +    $this->assertLink($nodes[2]->label());
    +    $this->assertLink($nodes[3]->label());
    +    $this->assertLink($nodes[4]->label());
    +    $this->assertLink($nodes[5]->label());
    +    $this->drupalGet('test-book/' . $nodes[6]->id());
    +    $this->assertLink($nodes[0]->label());
    +    $this->assertLink($nodes[1]->label());
    +    $this->assertLink($nodes[2]->label());
    +    $this->assertLink($nodes[3]->label());
    +    $this->assertLink($nodes[4]->label());
    +    $this->assertLink($nodes[5]->label());
    +    $this->assertLink($nodes[6]->label());
    +    $this->drupalGet('test-book/' . $nodes[7]->id());
    +    $this->assertLink($nodes[0]->label());
    +    $this->assertLink($nodes[1]->label());
    +    $this->assertLink($nodes[2]->label());
    +    $this->assertLink($nodes[3]->label());
    +    $this->assertLink($nodes[4]->label());
    +    $this->assertLink($nodes[5]->label());
    +    $this->assertLink($nodes[6]->label());
    +    $this->assertLink($nodes[7]->label());
    

    You should be able to create all those links using two nested for loops: for ($i = 0; $i < 8; $i++) { for ($j = 0; $j < $i; $j++) { } }

frob’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
40.38 KB
2.78 KB

Okay did that.

I am not sure what you are asking with #2 so I left that alone.

illeace’s picture

Status: Needs review » Reviewed & tested by the community

I repeated the same steps from #66 and also tried a few other Book-specific filters, like excluding book depth of 1. Everything I tested worked correctly for me. Looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 438f9e2 and pushed to 8.2.x. Thanks!

diff --git a/core/modules/book/src/Tests/Views/BookRelationshipTest.php b/core/modules/book/src/Tests/Views/BookRelationshipTest.php
index a26a19b..2302e8b 100644
--- a/core/modules/book/src/Tests/Views/BookRelationshipTest.php
+++ b/core/modules/book/src/Tests/Views/BookRelationshipTest.php
@@ -70,18 +70,6 @@ protected function createBook() {
     $this->book = $this->createBookNode('new');
     $book = $this->book;
 
-    /*
-     * Add page hierarchy to book.
-     * Book
-     *  |- Node 0
-     *   |- Node 1
-     *    |- Node 2
-     *     |- Node 3
-     *      |- Node 4
-     *       |- Node 5
-     *        |- Node 6
-     *         |- Node 7
-     */
     $nodes = array();
     // Node 0.
     $nodes[] = $this->createBookNode($book->id());

I just removed this - it is not that helpful and let's just avoid coding standards discussions about it.

  • alexpott committed 438f9e2 on 8.2.x
    Issue #1853524 by frob, jibran, eriknewby, olli, vijaycs85, xjm,...
jibran’s picture

Yay!!! this is fixed after 3 years.

PS: I still don't have a clue how we fixed #37.

frob’s picture

It looks like whatever error that was must have been fixed elsewhere. I don't know what test it was that failed because the page for the test summary doesn't exist anymore. Lets just be happy that this is done. :)

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -beta target

Was not actually committed to 8.1.x.

xjm’s picture

Issue tags: +8.2.0 release notes
yuseferi’s picture

it looks like a good idea and still in Drupal 9.3 there is no functionality to view over book items.