Support from Acquia helps fund testing for Drupal Acquia logo

Comments

brandenlhamilton’s picture

Assigned: Unassigned » brandenlhamilton
Dave.Ingram’s picture

I'm working on an initial patch for this.

Dave.Ingram’s picture

OK, 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.

Dave.Ingram’s picture

OK, 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.

Dave.Ingram’s picture

Status: Active » Needs review
FileSize
2.68 KB

Whoops. Marking needs review for testbot.

Crell’s picture

Status: Needs review » Active

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

Dave.Ingram’s picture

Status: Active » Needs review
FileSize
2.84 KB

OK, 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?

Status: Needs review » Needs work

The last submitted patch, user_view_page-1987898-7.patch, failed testing.

Dave.Ingram’s picture

Status: Needs work » Needs review
FileSize
2.84 KB

Rerolled against latest in 8.x. I think the failure above was just a hiccup in testbot though.

Status: Needs review » Needs work

The last submitted patch, user_view_page-1987898-9.patch, failed testing.

Crell’s picture

You 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.)

star-szr’s picture

Status: Needs work » Needs review
Issue tags: -WSCCI-conversion

#9: user_view_page-1987898-9.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, user_view_page-1987898-9.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.09 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, user-1987898-14.patch, failed testing.

tim.plunkett’s picture

Assigned: Dave.Ingram » Unassigned
Status: Needs work » Needs review
FileSize
4.03 KB

user_page getting in broke this patch, no interdiff possible.

But we shouldn't have been deleting that title callback.

Status: Needs review » Needs work

The last submitted patch, user-1987898-16.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.32 KB
383 bytes

There'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.

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, user-1987898-18.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#18: user-1987898-18.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, user-1987898-18.patch, failed testing.

unstatu’s picture

Assigned: Unassigned » unstatu

I am working on it.

Crell’s picture

unstatu: Any progress?

tim.plunkett’s picture

unstatu’s picture

Assigned: unstatu » Unassigned

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

h3rj4n’s picture

Assigned: Unassigned » h3rj4n

I'll give it a go.

h3rj4n’s picture

+++ b/core/modules/user/user.routing.yml
@@ -88,3 +88,14 @@ user_login:
+user_view_page:
+  pattern: '/user/{account}'
+  options:
+    converters:
+      account: 'user'
+  defaults:
+    _controller: '\Drupal\user\Controller\UserController::viewPage'
+  requirements:
+    account: \d

Why is this {account}?

h3rj4n’s picture

Status: Needs work » Needs review
FileSize
7.75 KB
1.35 KB

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

star-szr’s picture

Status: Needs review » Needs work

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

star-szr’s picture

@@ -49,7 +49,7 @@ function testBlockVisibility() {
-    $this->drupalGet('USER/' . $this->adminUser->id());
+    $this->drupalGet('user/' . $this->adminUser->id());
     $this->assertNoText($title, 'Block was not displayed according to block visibility rules regardless of path case.');

Well, 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.

h3rj4n’s picture

FileSize
1.86 KB

I don't have an interdiff because I removed a lot of files.

h3rj4n’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, user-1987898-31.patch, failed testing.

tim.plunkett’s picture

You need to remove that whole hunk from the BlockTest, we don't support case-insensitive routes.

h3rj4n’s picture

Status: Needs work » Needs review
FileSize
2.61 KB
767 bytes

Removed the test.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great!

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
@@ -49,9 +49,6 @@ function testBlockVisibility() {
-    $this->drupalGet('USER/' . $this->adminUser->id());
-    $this->assertNoText($title, 'Block was not displayed according to block visibility rules regardless of path case.');

Before we blatantly remove those tests, can we "git blame" and figure out when/why they were added?

+++ b/core/modules/user/user.module
@@ -1392,18 +1389,6 @@ function user_delete_multiple(array $uids) {
 /**
- * Page callback wrapper for user_view().
- */
-function user_view_page($account) {
-  if (is_object($account)) {
-    return user_view($account);
-  }
-  // An administrator may try to view a non-existent account,
-  // so we give them a 404 (versus a 403 for non-admins).
-  throw new NotFoundHttpException();

So what's the replacement for this code?

+++ b/core/modules/user/user.routing.yml
@@ -102,3 +102,11 @@ user_login:
+  requirements:
+    user: \d

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
356 bytes
2.61 KB

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

So what's the replacement for this code?

The code is basically replaced by the following lines of yml:

+  defaults:
+    _entity_view: 'user.full'
+  requirements:
+    user: \d+
+    _entity_access: 'user.view'

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

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.

Damn good catch! There is just one other place where we use the regex yet, and this is already using \d+

alexpott’s picture

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
@@ -49,9 +49,6 @@ function testBlockVisibility() {
-    $this->drupalGet('USER/' . $this->adminUser->id());
-    $this->assertNoText($title, 'Block was not displayed according to block visibility rules regardless of path case.');

Yep 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

URLs in general are case-sensitive (with the exception of machine names). There may be URLs, or parts of URLs, where case doesn't matter, but identifying these may not be easy. Users should always consider that URLs are case-sensitive.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

xjm’s picture

I'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.

alexpott’s picture

xjm’s picture

The test was added in: #368093: Block visibility settings are case sensitive ?. (Look who!) ;)

Letharion’s picture

Assigned: h3rj4n » Unassigned

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

Letharion’s picture

Re-rolled patch, test now kept.

Letharion’s picture

Issue tags: +LONDON_2013_AUGUST
Crell’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1987898-user-view-46.patch, failed testing.

dawehner’s picture

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

Letharion’s picture

Initially 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?

Letharion’s picture

With #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.

xjm’s picture

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

jibran’s picture

Status: Needs work » Needs review
Issue tags: -WSCCI-conversion, -LONDON_2013_AUGUST

#46: 1987898-user-view-46.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion, +LONDON_2013_AUGUST

The last submitted patch, 1987898-user-view-46.patch, failed testing.

damiankloip’s picture

Status: Needs work » Closed (duplicate)

Sorry, 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.