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 |
---|---|---|---|
#46 | 1987898-user-view-46.patch | 1.86 KB | Letharion |
#46 | interdiff_1987898_46.txt | 637 bytes | Letharion |
#38 | 1987898-user-view-38.patch | 2.61 KB | vijaycs85 |
#38 | 1987898-diff-35-38.txt | 356 bytes | vijaycs85 |
#35 | interdiff.txt | 767 bytes | h3rj4n |
Comments
Comment #1
brandenlhamilton CreditAttribution: brandenlhamilton commentedComment #2
Dave.Ingram CreditAttribution: Dave.Ingram commentedI'm working on an initial patch for this.
Comment #3
Dave.Ingram CreditAttribution: Dave.Ingram commentedOK, initial patch is pretty close, but 'title callback' is, so far as I can tell, not working right now. @see http://drupal.org/node/1981644
That said, here is the initial conversion to the new controller.
Comment #4
Dave.Ingram CreditAttribution: Dave.Ingram commentedOK, since title callback is not fully sorted, Crell said to just do this with drupal_set_title for now. With that said, I'm submitting a new patch for this controller which I believe is working well.
Comment #5
Dave.Ingram CreditAttribution: Dave.Ingram commentedWhoops. Marking needs review for testbot.
Comment #6
Crell CreditAttribution: Crell commenteduser_view() is now just a dumb wrapper around entity_view, which is just a dumb wrapper around getting the render controller:
http://api.drupal.org/api/drupal/core!modules!user!user.module/function/...
http://api.drupal.org/api/drupal/core!includes!entity.inc/function/entit...
At bare minimum we can inline what was entity_view() here, give or take injection.
Also, using drupal_set_title() in the controller is probably the best we can do at the moment, so go ahead and do that.
Comment #7
Dave.Ingram CreditAttribution: Dave.Ingram commentedOK, updating to use entity_view rather than user_view. I've also rerolled against the latest in 8.x. Not 100% sure what I would be injecting in this case.. entityManager?
Comment #9
Dave.Ingram CreditAttribution: Dave.Ingram commentedRerolled against latest in 8.x. I think the failure above was just a hiccup in testbot though.
Comment #11
Crell CreditAttribution: Crell commentedYou would inject the entity manager using create(), and then in the controller method do:
$this->entityManager->getRenderController($entity->entityType())->view($entity, $view_mode, $langcode);
(Which I just copied right out of entity_view, as it's the only line in that function.)
Comment #12
star-szr#9: user_view_page-1987898-9.patch queued for re-testing.
Comment #14
tim.plunkettRerolled.
Comment #16
tim.plunkettuser_page getting in broke this patch, no interdiff possible.
But we shouldn't have been deleting that title callback.
Comment #18
tim.plunkettThere's an issue with fit here. I think @dawehner has a patch for that somewhere, I'll have to find it. In the meantime, here's a hack.
Comment #20
tim.plunkett#18: user-1987898-18.patch queued for re-testing.
Comment #22
unstatu CreditAttribution: unstatu commentedI am working on it.
Comment #23
Crell CreditAttribution: Crell commentedunstatu: Any progress?
Comment #24
tim.plunkettThis should use #2040265: Add a _entity_view routing default for viewing an entity when its in.
Comment #25
unstatu CreditAttribution: unstatu commentedHi everybody,
My laptop has broken this week and I cannot advance on the patch. I set the issue as Unassign in case that someone wants to work on it.
Comment #26
h3rj4n CreditAttribution: h3rj4n commentedI'll give it a go.
Comment #27
h3rj4n CreditAttribution: h3rj4n commentedWhy is this {account}?
Comment #28
h3rj4n CreditAttribution: h3rj4n commentedUpdated the patch. The last tests that failed don't fail on my system. Let's see if the testbot can come up with anything.
I changed a test because, in my opinion, the test did something wrong. See interdiff for the change.
Comment #29
star-szr@h3rj4n - Thanks! The change you made to the test makes sense I think, but it is not related to the other changes in this issue and therefore out of scope.
If you want to update that test it would be best to make a separate issue.
Comment #30
star-szrWell, actually the test is correct - it's checking the block visibility regardless of path case, so please remove that change :)Edit: don't mind me, routes are case sensitive per @tim.plunkett on IRC.
Comment #31
h3rj4n CreditAttribution: h3rj4n commentedI don't have an interdiff because I removed a lot of files.
Comment #32
h3rj4n CreditAttribution: h3rj4n commentedComment #34
tim.plunkettYou need to remove that whole hunk from the BlockTest, we don't support case-insensitive routes.
Comment #35
h3rj4n CreditAttribution: h3rj4n commentedRemoved the test.
Comment #36
dawehnerGreat!
Comment #37
webchickBefore we blatantly remove those tests, can we "git blame" and figure out when/why they were added?
So what's the replacement for this code?
My regex is a little rusty, but shouldn't that be "\d+"? All of the examples on http://symfony.com/doc/current/book/routing.html#adding-requirements use that.
Otherwise, this'll probably work for user 1 but not user 1232313.
Comment #38
vijaycs85Here is where we added:
09655050 (webchick 2012-10-13 20:42:04 -0700 86) $this->drupalGet('USER');
24b70a28 (Dries 2012-09-25 08:41:28 -0400 87) $this->assertNoText($title, 'Block was not displayed according to block visibility rules regardless of path case.');
to fix #1811020: Decouple BlockTest from the node module and #1741338: Removing t() from asserts in simpletests in block module - do we need to rollback or do anything?
Seems we need to address NotFoundHttpException() as it is throwing 404 for all users.
Reg expression: Fixed.
Comment #39
dawehnerThe code is basically replaced by the following lines of yml:
_entity_view takes care that it is rendered via the entity system and _entity_access ensures the access checking.
For non existing accounts the upcasting ($uid => $user) code, will already throw an exception.
Damn good catch! There is just one other place where we use the regex yet, and this is already using \d+
Comment #40
alexpottYep removing this is weird. Basically with the new routing system urls are case sensitive. We can regard this as a feature or regression. This could cause quite a bit of pain for users upgrading from previous versions on Drupal.
According to http://www.w3.org/TR/WD-html40-970708/htmlweb.html
Comment #41
alexpottSetting back to needs work. At the very least the issue summary needs to be updated with why removing the test is ok and whether or not the decision to make paths case sensitive has actually been made.
Comment #42
xjmI'd like to see a reference to an issue on path case sensitivity, including if the upgrade path has been considered. That could wreak havoc on existing sites.
Comment #43
alexpottOpened a critical to discuss the case sensitivity issue #2075889: Make Drupal handle incoming paths in a case-insensitive fashion for routing
Comment #44
xjmThe test was added in: #368093: Block visibility settings are case sensitive ?. (Look who!) ;)
Comment #45
Letharion CreditAttribution: Letharion commentedSo the sentiment in #2075889: Make Drupal handle incoming paths in a case-insensitive fashion for routing seems to be that the paths should be case sensitive, thus the test makes sense as it is.
Comment #46
Letharion CreditAttribution: Letharion commentedRe-rolled patch, test now kept.
Comment #47
Letharion CreditAttribution: Letharion commentedComment #48
Crell CreditAttribution: Crell commentedComment #50
dawehnerI would like to mark this as duplicate of #2076085: Resolve the need for a 'title callback' using the route as it is done on there as well.
Comment #51
Letharion CreditAttribution: Letharion commentedInitially I figured why not, but now, since #2076085: Resolve the need for a 'title callback' using the route still has a discussion to resolve, I'm wondering if it would make sense for this issue to continue with what it was doing, and let the title issue focus on the title issue?
Comment #52
Letharion CreditAttribution: Letharion commentedWith #2076085: Resolve the need for a 'title callback' using the route not having moved yet, I'm gonna try to move forward with this.
Since in D7, one would get back a page from /USER, the test was built to ensure that the block rules were case sensitive.
Currently the test fails because Symfony doesn't like the /USER request, that URL doesn't exist, so we can't retrieve it to see if the block shows up.
The option I see are:
1) Consider the test nonse. We no longer support case insensitive paths, so this "should" not be an issue. Drop the test.
2) Implement /USER in the test module.
I favor the second option.
This depends on the outcome of #2075889: Make Drupal handle incoming paths in a case-insensitive fashion for routing.
Comment #53
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #54
jibran#46: 1987898-user-view-46.patch queued for re-testing.
Comment #56
damiankloip CreditAttribution: damiankloip commentedSorry, looks like #2076085: Resolve the need for a 'title callback' using the route is all done - with the user view conversion too. Closing this as a dupe now.