Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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):
- 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.
Comment | File | Size | Author |
---|---|---|---|
#91 | 90-inter.diff | 2.78 KB | frob |
#91 | reintroduce_views-1853524-90.patch | 40.38 KB | frob |
#89 | reintroduce_views-1853524-89.patch | 42.34 KB | frob |
Comments
Comment #2
Bojhan CreditAttribution: Bojhan commentedHow much of this is integration, seems bid etc are still missing? I dont think those are feature requests.
Comment #3
jibranreroll.
Comment #4
dawehnerI think it should be just "Provide views data for book.module."
I think these comments don't give you any information, but the other ones do.
... Should be contains.
Needs inheritdoc
You could inject the entity storage controller in order to load the node.
Comment #5
jibranFixed #4. Do we need tests?
Comment #6
dawehnerI fear we should have some.
Comment #7
jibranThe node storage controller.
Comment #8
dawehnerThat is easy to fix.
Comment #9
eriknewby CreditAttribution: eriknewby commentedAssigned via drupalmentoring.org - update following shortly.
Comment #10
eriknewby CreditAttribution: eriknewby commentedadded changes from #7 - updated docblock.
Comment #11
eriknewby CreditAttribution: eriknewby commentedForgot a period.
Comment #12
xjmThanks @eriknewby!
So next we just need those tests.
Comment #13
dawehnerLet's put everything in a new line, so it is easier to adapt later.
Comment #14
eriknewby CreditAttribution: eriknewby commentedHopefully I understood you correctly. Please see new patch attached.
Comment #15
dawehnerPerfect.
Comment #16
oadaeh CreditAttribution: oadaeh commentedChanging the tests tag to the more universally used one.
Comment #17
jibranit needs reroll after #1969388: Change notice: Add dedicated annotations for Views plugins.
Comment #18
Nitesh Sethia CreditAttribution: Nitesh Sethia commentedThis patch is working fine and there is no need for any rerolling.
Comment #19
jibranHi @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.
Comment #20
dawehnerThis also has to be adapted to load just a single node.
Comment #21
jibranFixed #7 #13, #17 and #20. inter diff is against #5.
Comment #22
vijaycs85Tested 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.
Comment #23
vijaycs85#21: drupal8.book-module.1853524-21.patch queued for re-testing.
Comment #24
xjmComment #25
xjmThis needs test coverage.
Comment #26
xjmComment #27
martin107 CreditAttribution: martin107 commented21: drupal8.book-module.1853524-21.patch queued for re-testing.
Comment #28
jibranFirst 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
Comment #29
jibranI have to re-write the
book_views_data
after #2084421: Phase 2 - Decouple book module schema from menu linksComment #30
jibranHere 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?
Comment #31
dawehnerThe 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.
Did we considered to use a placeholder in the string instead?
This should point to the same class...
Did we considered to point to the dedicated NodeStorageInterface?
Comment #32
jibranThanks for the review and these are all very good points. I have fixed all of them. I am working on a test view.
Comment #33
jibranThanks @dawehner for the pointers and @ClemensTolboom for all the support.
Comment #34
clemens.tolboomIgnore 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()
Don't call this Root (its too short). Maybe TopLevelNode or BookRoot or RootNode
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.Comment #36
damiankloip CreditAttribution: damiankloip commentedThis is TRUE by default.
Just call this $label or something.
Comment #37
jibranFixed #34.2 and #36
I have no idea how to fix that. I tried creating relationship plugin but nothing worked. Need some help here.
Comment #39
dawehnerMeh.
Comment #40
jibranWhat?
Comment #41
dawehnerIts sad that this issue wasn't been able to get in, no offence.
Comment #42
jibranDo we have a solution for this yet?
Comment #43
jibranI think we can re-roll this patch now.
Comment #44
vijaycs85The patch applies without any conflict. We might need to address test fails and #37
Comment #45
olli CreditAttribution: olli commentedI 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:).
Comment #46
olli CreditAttribution: olli commentedI tried to use something like this
, but that didn't work.
This makes sense to me because the relationship is not required but it feels wrong to change this here.
Comment #47
jibranWoW @olli impressive work. If I'd find you on IRC i'd give you 1000(oili+=1000) karma points for this.
Comment #49
olli CreditAttribution: olli commentedThanks @jibran +1000 to you too.
Here's a patch with an isset around that exception and an interdiff with more context.
Comment #50
jibranI heard @larowlan saying a lot of times.
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
andEntityFieldRenderer
. 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.Unrelated.
This is very good fix imo. I don't think we need tests for this. What do you think?
Seems legit as well. Again, do we need test coverage for this?
And again do we need test coverage for this one as well?
Comment #51
dawehnerIt is so good to so some progress here!
Do we really need an ! placeholder there?
Did you considered to write that test with two nested for looops like (danger, pseudo code!!) :
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?
Comment #52
olli CreditAttribution: olli commentedFiled #2534780: Fatal error rendering fields using an optional relationship to fix that.
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?
Should this implement some cache related interface and can we use 'route.book_navigation' cache context?
Comment #53
dawehnerMh, true. This doesn't really make sense.
Oh yeah good point. That context is basically what the view will vary by!
Comment #54
olli CreditAttribution: olli commentedReroll after #2534780: Fatal error rendering fields using an optional relationship.
Fixed #50.1.
Comment #56
jhedstromSince 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.
Comment #59
xjmThis will need to be against 8.1.x now.
Comment #60
frobI 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?
Comment #62
frobI fixed the broken !parent to be @parent. This should allow the tests to pass.
Comment #63
frobAgh, crying children distracting me. Marking needs review.
Comment #64
raj45 CreditAttribution: raj45 commentedIt 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?
Comment #65
frobWhile 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.
Comment #66
illeace CreditAttribution: illeace at Clarity Innovations, Inc. commentedTo test this I:
Comment #67
vijaycs85Addressing #56. Though it's a minor, traits reduce the duplicate method.
Comment #69
frob@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.
Comment #70
frobI 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.
Comment #71
vijaycs85@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.
Comment #73
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedA shame, it looks like this just missed out on 8.1. Would it be disruptive?
Comment #74
frobI 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.
Comment #75
alexpottAbove are a few coding standards fixes to fix.
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.
Comment #76
martin107 CreditAttribution: martin107 as a volunteer commentedJust 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.
Comment #77
frobThis 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.
Comment #78
alexpottComment #80
frobOkay, 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.
Comment #81
frobHere is the latest roll, although I am not sure exactly what to do about the comment:
This will never pass coding standards. However, there are lots of coding standards issues in the book module currently.
Comment #83
frobOkay, 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.
Comment #84
xjmThis 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.)
Comment #85
frobI have rerolled the patch from 62 to include the stuff from 75.
Comment #86
frobComment #87
frobComment #88
alexpottWe never actually test the book. We also don't test what happens when a node id that is not a book is an argument.
Comment #89
frobI 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.
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.
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.
Comment #90
dawehnerThis patch looks pretty ready!
@file should be dropped now.
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.
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++) { } }
Comment #91
frobOkay did that.
I am not sure what you are asking with #2 so I left that alone.
Comment #92
illeace CreditAttribution: illeace at Clarity Innovations, Inc. commentedI 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.
Comment #93
alexpottCommitted 438f9e2 and pushed to 8.2.x. Thanks!
I just removed this - it is not that helpful and let's just avoid coding standards discussions about it.
Comment #95
jibranYay!!! this is fixed after 3 years.
PS: I still don't have a clue how we fixed #37.
Comment #96
frobIt 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. :)
Comment #98
xjmWas not actually committed to 8.1.x.
Comment #99
xjmComment #100
yuseferi CreditAttribution: yuseferi commentedit looks like a good idea and still in Drupal 9.3 there is no functionality to view over book items.