See #1971384: [META] Convert page callbacks to controllers

CommentFileSizeAuthor
#10 picture-mapping-1983844-10.patch9.92 KBtim.plunkett
#10 interdiff.txt1.51 KBtim.plunkett
#7 drupal-1983844-7.patch8.41 KBdawehner
#6 picture-1983844-6.patch8.23 KBtim.plunkett
#6 interdiff.txt729 bytestim.plunkett
#2 picture-1983844-1.patch8.18 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
Issue tags: +WSCCI-conversion
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
picture-1983844-1.patch8.18 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: Unassigned » tim.plunkett
aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented 
+++ b/core/modules/picture/lib/Drupal/picture/PictureController.phpundefined
@@ -0,0 +1,50 @@
+   * @param \Drupal\Core\Entity\EntityManager $entity_manager

Hmm 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!

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Needs work

Looked at some other patches. We still need the "Constructs ..." in the constructor.

:)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
interdiff.txt729 bytes
picture-1983844-6.patch8.23 KB

Ah yes, I was missing that. Also a leading slash

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
drupal-1983844-7.patch8.41 KB 
+++ b/core/modules/picture/lib/Drupal/picture/PictureController.phpundefined
@@ -0,0 +1,52 @@
+class PictureController implements ControllerInterface {

Couldn't we have a generic entity list controller which just gets the entity type as well? Here is a patch for that.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Good idea! That will be useful in at least 8 other places.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Let's open new follow ups once that's in.

I guess with this change we need a proper test for this special entity controller?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
interdiff.txt1.51 KB
picture-mapping-1983844-10.patch9.92 KB

A 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.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

That's looking great!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Convert picture.module callbacks to routes/controllers » Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case

Retitling...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case » Change notice: Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed a57952c and pushed to 8.x. Thanks!

I guess a change notice needs to be updated.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned

I had four change notice issues assigned to me but YesCT told me I could give away two of them. Sorry this one.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Postponed

Actually, #2008356: Provide a _entity_list route default to replace Controller\EntityListController and mimic _entity_form might make this entire issue obsolete.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

@tim.plunkett: that was comitted. But we still need/want to have Drupal\Core\Entity\Controller\EntityListController documented in the updates, no?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Well you we can still document that class, but you don't actually use it directly, you use _entity_list in the route...

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Change notice: Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case » Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case
Status: Postponed » Fixed
Issue tags: -Needs change record

Then.

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

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Component: picture.module » responsive_image.module
Issue summary: View changes