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.
Part of #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#31 | 1987858-30.patch | 13.47 KB | jibran |
#31 | interdiff.txt | 462 bytes | jibran |
#25 | interdiff.txt | 642 bytes | tim.plunkett |
#25 | taxonomy-1987858-25.patch | 13.92 KB | tim.plunkett |
#23 | taxonomy-1987858-23.patch | 13.91 KB | tim.plunkett |
Comments
Comment #1
oenie CreditAttribution: oenie commentedI'll give this conversion a shot.
Comment #2
oenie CreditAttribution: oenie commentedGot a patch that conforms to the WSCCI conversion guide, but the autocomplete JSON result doesn't show up when searching.
Any pointers/thoughts ?
Comment #3
oenie CreditAttribution: oenie commentedNote to self: never assume that all normal strings are automagically transformed into a Drupal\Core\TypedData\String object in a Controller.
Comment #4
dawehnerJust in general: do you think it's worth to convert the autocompletion once entity reference should be able to deal with it already?
Let's keep this in one line ...
It's not really a callback anymore.
Missing documentation for the $request variable.
Can't we return a proper response with just the text in there?
Just put an empty line before the end of the class.
Comment #5
oenie CreditAttribution: oenie commentedCorrecting this patch although it might become obsolete because of entity autocomplete.
Comment #6
oenie CreditAttribution: oenie commentedTurns out that this is planned to be rewritten anyway, so maybe wait for a decision on issue #1169964 : Taxonomy autocomplete: decouple from Field API and factor out a helper so that code is reusable
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedThis issue is part of a critical meta issue, so let's not hold it up on the other issue, which doesn't seem to be making much progress anyway.
Comment #8
oenie CreditAttribution: oenie commentedRerolled.
Comment #9
dawehnerThis looks nearly ready!
Should there also be a 403 status code?
There is Tags::explode and Unicode:strtolower which better should be used here.
Let's use String::checkPlain
Comment #10
oenie CreditAttribution: oenie commentedComment #11
dawehnerIt is really odd to have symfony and drupal mixed up.
field_info_field($field_name) should be directly replaced by an injected 'field.info'->getField($field_name) method call. In general try to have a look at all the procedural functions if you port something.
I am wondering whether you have tried to port this to a entity query (alias entity field query in D7)
Tags::implode()
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedAll WSCCI conversions are critical, but we're not yet setting their priority as such. Setting this one though because of #2056627: Form API autocomplete is broken for routes. Because of that issue, I'd love to see this one get in quickly, even if a follow up is needed to finish noncritical details.
Comment #13
jibranFixed everything from #11.
Comment #14
dawehnerPerfect!
Comment #16
jibranFixed the test failure ;)
Comment #17
dawehnerBack to green.
Comment #18
dawehnerNope, see https://drupal.org/node/2056627#comment-7767543
Comment #19
oenie CreditAttribution: oenie commentedI'm not sure whether my input is still relevant here. Removing myself as assignee.
Comment #20
jibran#16: 1987858-16.patch queued for re-testing.
Comment #22
tim.plunkettWorking on this, I noticed a couple odd things.
Comment #23
tim.plunkettOkay, fixed the three usages of it, and fixed some coding standards.
Manually tested all scenarios.
Comment #24
dawehnerI doubt :p
I guess both the route name and the route parameters have to be configurable to make sense? No idea to be hoenst.
I guess the d7 content sript is for d7?
Comment #25
tim.plunkettgenerate-d7-content.sh has been updated to use entity_create and similar, so I think it's a lost cause.
The route params don't need to be configurable, they weren't before, they were just appended to the path.
Comment #26
jibranChanges looks good to me and it is green. RTBC from me.
Comment #27
dawehnerEven other people converted other code, there is no reason to convert it here, while knowing it is wrong.
Beside from that I am wondering whether we need an upgrade path for the settings.
Comment #28
jibranoh we are missing one more change quoting @tim.plunkett from #2056627-22: Form API autocomplete is broken for routes
Comment #29
jibranJust talked to @dawehner let's move
#autocomplete_path
removal to follow up. NW for #27.Comment #30
jibranAdded #2071115: Remove #autocomplete_path in favor of #autocomplete_route.
Comment #31
jibranFixed #27.
Comment #32
dawehnerThis is good to go now.
Comment #33
alexpottCommitted 499080f and pushed to 8.x. Thanks!