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 |
---|---|---|---|
#36 | interdiff.txt | 4.08 KB | tim.plunkett |
#36 | non-test-non-form-2089635-36.patch | 120.6 KB | tim.plunkett |
#28 | non-test-non-form-2089635-28.patch | 120.41 KB | tim.plunkett |
#28 | interdiff.txt | 3.88 KB | tim.plunkett |
#26 | non-test-non-form-2089635-26.patch | 120.25 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettGot about halfway done, going out to dinner. I know I broke at least one thing...
Anything with an access check or logic in the hook_menu is still non-trivial.
Comment #3
tim.plunkettOkay, I'm back. Here are some attempted text fixes for the last run, while I work on the rest.
Comment #5
tim.plunkettLOL I did not mean to RTBC.
Here's one I did since then.
Comment #7
tim.plunkettComment #9
tim.plunkettStill missing:
#1987882: Convert content_translation routes to a new style controller
#1972990: Convert tracker_page() to a Controller
#1978992: Convert translation_node_overview() to a Controller
And #2027115: Allow views to override existing routing items is a huge blocker for these two:
#2021161: Replace the fallback node listing with a list controller
#1938884: Replace the fallback user listing with a list controller
If this passes, I would like to propose it be reviewed and committed, and those 5 issues be handled independently, as they are much more complex.
Comment #11
tim.plunkettMore fixes.
Comment #13
tim.plunkettDon't refactor after running tests without rerunning the tests.
Comment #15
tim.plunkettFix menu_get_object, add @deprecated, and remove accidental changes to symfony
Comment #17
tim.plunkettThree of the remaining failures are related to search queries with / in them.
For example,
I'm not sure how to best resolve that.
Noooo idea about Drupal\translation\Tests\TranslationTest.
If anyone wants to pitch in, feel free.
Comment #18
tim.plunketthttp://drupalcode.org/sandbox/tim.plunkett/1698392.git/shortlog/refs/hea... if you're feeling awesome
Comment #19
disasm CreditAttribution: disasm commentedThis should fix search. Need to specify that the param can contain odd characters in requirements: http://symfony.com/doc/current/cookbook/routing/slash_in_parameter.html
Also, default search was taking plugin_id as param, it should be taking just keys and passing NULL as plugin_id.
Content translations a bit trickier, because none of the callbacks are in that module.
Comment #20
disasm CreditAttribution: disasm commentedforgot to fix path in search.routing.yml.
Comment #22
disasm CreditAttribution: disasm commentedper #9 tackling content_translation in a separate issue: #2090717: Convert content_translation routes to interim wrapper controller..
Comment #23
larowlanShould fix search
Comment #25
tim.plunkettThanks so much for that, @disasm and @larowlan!
Here's the aggregated interdiff of your changes, in case anyone is curious.
I think I figured out the last fail, will have a patch soon.
Comment #26
tim.plunkettSince @disasm got #2090717: Convert content_translation routes to interim wrapper controller. working already, merging that in.
This also now includes translation_node_overview.
So, the leftovers are:
tracker/%user_uid_optional
, and because people wanted it to be a view...Comment #27
dawehnerLet's ensure to add the title.
This one does not care about a drupal html page, so it should use _controller.
Just wondering whether the comment ID maybe cared about the pager before?
This should be also copied to the routing file.
Wasn't the approach in the issue way simpler?
At least node/add could have a title.
I can't see a reason why this should be not _content.
_content
Comment #28
tim.plunkettThe issue for dblog_top('search') (admin/reports/search) is way easier because it completely bypasses the module_exists :) It just always provides the route, which is wrong.
- $this->drupalGet('node/' . $this->node->id() . '/' . $comment->id());
did absolutely nothing anyway.I addressed all of the other issues.
Comment #29
dawehnerI assume this won't break the tests.
Comment #30
tim.plunkettFollow-ups:
#2091399: [META] Remove menu_get_object()
#2091411: Provide an easier mechanism for a route definition wrapped by module_exists()
Comment #32
tim.plunkettThe testbot had an apache segfault.
Requeueing.
Comment #33
webchickA-W-E-S-O-M-E! :D
I tried really hard to get through this whole patch but ran smack into babysitting duty, so only was able to give a cursory overview to the last, say, 2/3. Here's what I found so far though.
Yeah, wow. No idea what's happening here but that seems crazy. We need to document this, along with a @todo that tells us how to clean this up after.
Cross-referenced @deprecated and @todo! That's great!
Why does this "use" Request? It's not referenced below.
That sounds a bit too generic, like it would contain methods for all block admin-related thingies. Better as BlockDemoController or so?
In IRC, corrected: "BlockController."
Hm. That seems to have changed the nature of that test? Before it was testing for the comment permalink/canonical URL/whatever worked, not just that the comment was on the node page.
Berdir found #296486: TestingParty08: viewing an individual comment where the tests were added which confirms this. So either we should put this capability back (if we can track down why it was done and the reason remains valid) or we should remove these tests entirely.
This one is "ContactPageController" and does not extend ControllerBase.
We should probably convert these all in a consistent way.
and here is ModuleController, no base class once again.
The @todos need to be updated here.
Comment #34
BerdirThat drupalGet() does nothing because that functionality has been removed in #350984: Kick comment rendering out of node module, the tests for it were added only three months before that in #296486: TestingParty08: viewing an individual comment and after that was removed, simply pointed to the normal node page and happily testing the output there.
Comment #35
BerdirCrosspost.
Comment #36
tim.plunkettThose three dblog methods all call through to dblog_top(), that's not a copy/paste mistake.
Yes, @Berdir's code anthropology is correct, but I don't think we should remove the coverage. It's still good to assert that the comments are displayed below the node when they're supposed to be.
I resolved the other points @webchick made.
EDIT: The interdiff didn't include the changes to BlockAdminController, which was renamed to BlockController, and the stray
use Symfony\Component\HttpFoundation\Request;
was removed.Comment #37
larowlanIssues addressed from #33
There's a few that don't use ControllerBase (because they don't need to yet).
Happy to keep the test for the comment rendering on node/% even though comment-permalinks are of a different format now.
Comment #38
webchickAwesome. Can't find any other complaints, and this is a HUGE step forward in removing hook_menu() and friends. Let's go!
Committed and pushed to 8.x. YEAHH!
1 down, 3 to go. :)
Comment #39
Gábor HojtsyThis in combination with #2089635: Convert non-test non-form page callbacks to routes and controllers and #2032535: Resolve 'title' using the route and render array regressed contact form titles. See #2093027: Regression: contact category titles not used for page title anymore with a suggested fix.