Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This method is currently living in our View storage class, it doesn't really belong there. The storage class doesn't need to care about plugin managers or anything like that. This is only used in Views UI, specifically in the list controller.
Let's move it there. I have currently just commented out the (random) test assertion in ViewStorageTest for this. Not sure we have a test specifically for the listing controller?
Comment | File | Size | Author |
---|---|---|---|
#20 | deper.png | 37.71 KB | dawehner |
#20 | vdc-2012882-20.patch | 10.17 KB | dawehner |
#20 | interdiff.txt | 1.64 KB | dawehner |
#16 | 2012882-16.patch | 10.22 KB | damiankloip |
#16 | interdiff-2012882-16.txt | 1.03 KB | damiankloip |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedComment #2
tim.plunkettThis can be protected now
We can inject this here!
Oh come on :) Scope creep!
Comment #3
damiankloip CreditAttribution: damiankloip commentedHa :) How about this then? I'm not sure about injecting the storage controller? I guess we have to just get it from the container like that? As createInstance doesn't have knowledge that the actual EntityManager has called it.
Comment #4
tim.plunkettAny reason to not just call parent::__construct() I don't care much either way
Seems you forgot to actually use it?
Comment #6
damiankloip CreditAttribution: damiankloip commentedWhoops, missed out a use. Also made changes from feedback in #4, thanks.
EDIT: I did not call the parent, as that calls entity_get_info() in it's constructor. We can inject that now.
Comment #7
dawehnerMissing @param documentation
That's what I don't really get is how to test such functionality. This is clearly a method which could be proper unit tested, but it's marked as protected
Comment #8
damiankloip CreditAttribution: damiankloip commentedHmm, not sure. I guess we could only test it in the UI :/ If only we had some way to test protected methods or something.... ;) (Do you remember that issue?)
Comment #9
dawehnerHere is a unit test for this piece. This also fixes a small issue in the actual code, as it uses render arrays now.
Comment #11
damiankloip CreditAttribution: damiankloip commentedI guess we might have to do this for now?
Comment #13
dawehnerThis should fix it.
Comment #15
damiankloip CreditAttribution: damiankloip commentedI guess we both had the same change for that.
Comment #16
damiankloip CreditAttribution: damiankloip commentedhmm, SO something has changed recently that breaks table cells being rendered with drupal_render() ?!! I'm sure this worked once upon a time.
Comment #18
tim.plunkett#16: 2012882-16.patch queued for re-testing.
Comment #20
dawehnerWe have to go deeper!
Note: This is an interdiff against #13
Comment #21
damiankloip CreditAttribution: damiankloip commentedOf course!!
Comment #22
tim.plunkettNice fix @dawehner!
Comment #23
alexpottCommitted a544e13 and pushed to 8.x. Thanks!