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.
Comment | File | Size | Author |
---|---|---|---|
#10 | picture-mapping-1983844-10.patch | 9.92 KB | tim.plunkett |
#10 | interdiff.txt | 1.51 KB | tim.plunkett |
#7 | drupal-1983844-7.patch | 8.41 KB | dawehner |
#6 | picture-1983844-6.patch | 8.23 KB | tim.plunkett |
#6 | interdiff.txt | 729 bytes | tim.plunkett |
Comments
Comment #1
tim.plunkettComment #2
tim.plunkettComment #3
tim.plunkettComment #4
aspilicious CreditAttribution: aspilicious commentedHmm is this the standard for constructors in controllers? Document the additional params? Is this documented somewhere?
Reviewed the code carefully, to educate myself and I think this looks perfect...
I'm not confident enough yet to rtbc this. Great job Tim!
Comment #5
aspilicious CreditAttribution: aspilicious commentedLooked at some other patches. We still need the "Constructs ..." in the constructor.
:)
Comment #6
tim.plunkettAh yes, I was missing that. Also a leading slash
Comment #7
dawehnerCouldn't we have a generic entity list controller which just gets the entity type as well? Here is a patch for that.
Comment #8
tim.plunkettGood idea! That will be useful in at least 8 other places.
Comment #9
dawehnerLet's open new follow ups once that's in.
I guess with this change we need a proper test for this special entity controller?
Comment #10
tim.plunkettA note for committers, here's reason we're not breaking this up into a new issue:
#1947432-46: Add a generic EntityAccessCheck to replace entity_page_access()
#1945564-9: Convert confirm_form() in shortcut to the new form interface and convert route
Last time we added something new, it came with an implementation. So here it is with switching our test coverage to the routing system, and using this controller.
Comment #11
dawehnerThat's looking great!
Comment #12
alexpottRetitling...
Comment #13
alexpottCommitted a57952c and pushed to 8.x. Thanks!
I guess a change notice needs to be updated.
Comment #14
tim.plunkettI had four change notice issues assigned to me but YesCT told me I could give away two of them. Sorry this one.
Comment #15
tim.plunkettActually, #2008356: Provide a _entity_list route default to replace Controller\EntityListController and mimic _entity_form might make this entire issue obsolete.
Comment #16
Gábor Hojtsy@tim.plunkett: that was comitted. But we still need/want to have Drupal\Core\Entity\Controller\EntityListController documented in the updates, no?
Comment #17
tim.plunkettWell you we can still document that class, but you don't actually use it directly, you use _entity_list in the route...
Comment #18
Gábor HojtsyThen.
Comment #20
Eli-T