Part of #1971384: [META] Convert page callbacks to controllers

Convert these page callback to a new-style Controller, using the instructions on http://drupal.org/node/1800686 . They can share a single controller class, which takes the flood control system and config system as dependencies.

contact_flood_control() can then become a protected method on that class, using those object variables.

Files: 
CommentFileSizeAuthor
#147 contact-page-1938390-147.patch17.86 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,077 pass(es).
[ View ]
#139 interdiff.txt981 bytesandypost
#139 drupal8.contact-module.1938390-139.patch17.21 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,266 pass(es).
[ View ]
#137 drupal8.contact-module.1938390-137.patch16.72 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,688 pass(es), 0 fail(s), and 8 exception(s).
[ View ]
#135 drupal8.contact-module.1938390-135.patch16.89 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,847 pass(es).
[ View ]
#133 interdiff.txt1.92 KBandypost
#133 drupal8.contact-module.1938390-133.patch16.96 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,946 pass(es).
[ View ]
#125 interdiff.txt1.35 KBandypost
#125 drupal8.contact-module.1938390-125.patch16.88 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,349 pass(es).
[ View ]
#116 interdiff.txt667 bytesandypost
#116 drupal8.contact-module.1938390-116.patch16.88 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,073 pass(es).
[ View ]
#113 interdiff.txt1.16 KBandypost
#113 drupal8.contact-module.1938390-112.patch16.9 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,810 pass(es).
[ View ]
#111 interdiff.txt4.04 KBandypost
#111 drupal8.contact-module.1938390-110.patch17.68 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,757 pass(es), 11 fail(s), and 0 exception(s).
[ View ]
#108 interdiff.txt2.17 KBandypost
#108 drupal8.contact-module.1938390-108.patch18.5 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,325 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#107 drupal8.contact-module.1938390-107.patch16.85 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,433 pass(es).
[ View ]
#104 drupal8.contact-module.1938390-104.patch22.78 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,185 pass(es).
[ View ]
#102 interdiff.txt1.9 KBandypost
#102 drupal8.contact-module.1938390-102.patch22.75 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,843 pass(es).
[ View ]
#100 interdiff.txt4.42 KBandypost
#100 drupal8.contact-module.1938390-100.patch22.7 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,140 pass(es).
[ View ]
#98 drupal8.contact-module.1938390-98.patch22.7 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 59,293 pass(es).
[ View ]
#98 interdiff.txt2.1 KBjibran
#95 drupal8.contact-module.1938390-95.patch22.7 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,793 pass(es).
[ View ]
#95 interdiff.txt3.13 KBjibran
#93 drupal8.contact-module.1938390-93.patch19.57 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 59,135 pass(es), 11 fail(s), and 0 exception(s).
[ View ]
#93 interdiff.txt5.37 KBdisasm
#90 interdiff.txt17.98 KBandypost
#90 contact-page-1938390-90.patch20.38 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,792 pass(es), 11 fail(s), and 0 exception(s).
[ View ]
#85 drupal8.contact-module.1938390-85.patch21.02 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,326 pass(es), 96 fail(s), and 18 exception(s).
[ View ]
#85 interdiff.txt5.17 KBdisasm
#83 drupal8.contact-module.1938390-83-combined.patch30.38 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#81 drupal8.contact-module.1938390-81.patch21.95 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#81 interdiff-changes-80.txt6.36 KBdisasm
#78 drupal8.contact-module.1938390-78.patch26.13 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,233 pass(es), 159 fail(s), and 3 exception(s).
[ View ]
#78 interdiff.txt3.81 KBdisasm
#76 drupal8.contact-module.1938390-76.patch26.03 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 53,022 pass(es), 363 fail(s), and 323 exception(s).
[ View ]
#76 interdiff.txt936 bytesdisasm
#75 drupal8.contact-module.1938390-75.patch26.05 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#75 interdiff.txt1.16 KBdisasm
#71 interdiff.txt4.9 KBandypost
#71 contact-page-1938390-hacks.txt2.26 KBandypost
#71 contact-page-1938390-71.patch26.02 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,165 pass(es).
[ View ]
#69 interdiff.txt2.92 KBandypost
#69 contact-page-1938390-69.patch25.81 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,963 pass(es).
[ View ]
#67 interdiff.txt3.59 KBandypost
#67 contact-page-1938390-67.patch23.55 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,832 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#66 interdiff.txt4.29 KBandypost
#66 interdiff-51vs66.txt13.83 KBandypost
#66 contact-page-1938390-66.patch23.1 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,823 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#65 interdiff-51vs63.txt14.99 KBandypost
#64 interdiff.txt5.97 KBandypost
#64 contact-page-1938390-63.patch24.32 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,914 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#62 interdiff.txt1.24 KBandypost
#62 contact-page-1938390-62.patch25.11 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,532 pass(es), 30 fail(s), and 8 exception(s).
[ View ]
#57 interdiff.txt2.45 KBandypost
#57 contact-page-1938390-56.patch24.54 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#55 interdiff.txt17.34 KBandypost
#55 contact-page-1938390-55.patch24.66 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#51 contact-1938390-51.patch16.03 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,810 pass(es), 45 fail(s), and 0 exception(s).
[ View ]
#51 interdiff.txt750 bytesdawehner
#47 contact-1938390-47.patch16.01 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#47 interdiff.txt11.16 KBdawehner
#43 drupal-contact_forms-1938390-43.patch15.6 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 57,471 pass(es).
[ View ]
#43 interdiff.txt3.27 KBpfrenssen
#40 drupal-contact_forms-1938390-40.patch15.47 KBygerasimov
PASSED: [[SimpleTest]]: [MySQL] 57,165 pass(es).
[ View ]
#40 drupal-contact_forms-1938390-40-interdiff.patch7.62 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-contact_forms-1938390-40-interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#35 drupal-contact_forms-1938390-35.patch15.14 KBygerasimov
FAILED: [[SimpleTest]]: [MySQL] 57,136 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#35 drupal-contact_forms-1938390-35-interdiff.txt11.24 KBygerasimov
#28 drupal-contact_forms-1938390-28.patch13.87 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 57,872 pass(es), 126 fail(s), and 51 exception(s).
[ View ]
#28 interdiff.txt1.38 KBdisasm
#26 drupal-contact_forms-1938390-26.patch14.47 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 57,904 pass(es), 125 fail(s), and 51 exception(s).
[ View ]
#26 interdiff.txt844 bytesdisasm
#22 drupal-contact_forms-1938390-22.patch14.32 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 54,446 pass(es), 81 fail(s), and 34 exception(s).
[ View ]
#16 drupal-contact_forms-1938390-16.patch14.17 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#16 interdiff.txt543 bytesdisasm
#14 drupal-contact_forms-1938390-13.patch14.15 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#14 interdiff.txt4.65 KBdisasm
#10 drupal-contact_forms-1938390-10.patch13.94 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#10 interdiff.txt687 bytesdisasm
#9 drupal-contact_forms-1938390-9.patch13.93 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#9 interdiff.txt681 bytesdisasm
#7 drupal-contact_forms-1938390-7.patch13.93 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#7 interdiff.txt5.72 KBdisasm
#4 drupal-contact_forms-1938390-4.patch13.04 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 52,579 pass(es), 12 fail(s), and 2 exception(s).
[ View ]

Comments

Title:Convert contact_site_page to a new-style ControllerConvert contact_site_page and contact_person_page to a new-style Controller

Assigned:Unassigned» disasm

Status:Active» Needs review
StatusFileSize
new13.04 KB
FAILED: [[SimpleTest]]: [MySQL] 52,579 pass(es), 12 fail(s), and 2 exception(s).
[ View ]

Attaching patch of what I did so far. Marking needs review so others can see the testbot failures. It's failing to load /contact with no category parameter. contact_category is set to 0 by default in contact.routing.yml.

Status:Needs review» Needs work

The last submitted patch, drupal-contact_forms-1938390-4.patch, failed testing.

Issue tags:+needs security review

+++ b/core/modules/contact/contact.module
@@ -97,31 +97,23 @@ function contact_menu() {
+    'route' => 'contact_site_page',

route_name

+++ b/core/modules/contact/contact.module
@@ -97,31 +97,23 @@ function contact_menu() {
+    'route' => 'contact_personal_page',

route_name

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.php
@@ -0,0 +1,67 @@
+    // Anonymous users cannot have contact forms.
+    if (!$account->uid) {
+      return FALSE;

This could easily get spun off to a separate access checker.

_anonymous: TRUE

If set to true, only the anon user can access it. If set to FALSE, anyone BUT the anonymous user can access it.

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.php
@@ -0,0 +1,67 @@
+    // User administrators should always have access to personal contact forms.
+    if (user_access('administer users')) {
+      return TRUE;

Because of the way access checkers stack, this can just be a _permission check. If a person has that permission, they pass. If not, return NULL and let other checkers do their thing.

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.php
@@ -0,0 +1,67 @@
+    // If requested user has been blocked, do not allow users to contact them.
+    if (empty($account->status)) {
+      return FALSE;

I wonder if this could be its own checker? Maybe, or maybe just part of what's left of the ContactPageAccess checker.

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.php
@@ -0,0 +1,67 @@
+    $account_data = drupal_container()->get('user.data')->get('contact', $account->id(), 'enabled');

That service can/should be injected into the access checker. Easy service to add.

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.php
@@ -0,0 +1,67 @@
+    elseif (!config('contact.settings')->get('user_default_enabled')) {
+      return FALSE;

This should also get the config manager injected.

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.php
@@ -0,0 +1,67 @@
+    return user_access('access user contact forms');

Again, stackable permission checker.

Although, that does raise an interesting question of how we handle multiple permissions. Maybe the permission checker needs to allow multiple permissions, and accept any? Do we need a marker for all vs. any? Hm...

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.php
@@ -0,0 +1,131 @@
+  protected $flood;

Needs docblock.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.php
@@ -0,0 +1,131 @@
+  public function contactPersonalPage($user) {
+    $recipient = $user;
+    global $user;

Oh dear oh dear. This is going to be a problem indeed, if global $user is going to always collide with parameters... Not sure what to do here. I'd like to have someone from the security team weigh in, because this is going to be an issue everywhere until we eliminate global $user.

Can we make eliminating global $user a critical security issue, which therefore puts pressure on the Symfony session conversion? :-)

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.php
@@ -0,0 +1,131 @@
+      $this->contactFloodControl();

What we should probably do here is have contactFloodControl() return a boolean, then do the actual error setting and exception throwing in the method itself. That subjectively feels cleaner, because it puts the IO in the controller method rather than in a logic method. Same above.

Of course, if we can figure out how to move configurable flood control to the access system, that would be even better.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.php
@@ -0,0 +1,131 @@
+  $config = config('contact.settings');

Config manager should be an injected dependency so that we don't need to call config().

Status:Needs work» Needs review
Issue tags:-needs security review
StatusFileSize
new5.72 KB
new13.93 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

I've made all the requested changes other than stacking the access checks. The way the checks are written, it's very hierarchical, for example, an admin overrides all the other checks. Also, the anonymous user in this case isn't the poster, but the account being posted too, so breaking it out into a user access check isn't viable either.

Issue tags:+needs security review

StatusFileSize
new681 bytes
new13.93 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

attached fixes argument on ContactBundle

StatusFileSize
new687 bytes
new13.94 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Third times a charm I hope!

Hm. Instead of asking for a security review, we should have a (web) test that verifies all of the existing permission permutations in HEAD, so we do not need a security review.

Apparently, that seems to be the whole point and sole purpose of the ContactPersonalTest (which thus lacks "Access" in its name). :)

That said, I agree with @disasm that the access checks performed there are fairly specific, and the order in which the checks are being performed is of importance, too. Trying to resemble that in _pseudo: _sub: ORs, and _ANDs would only result in an array(#form => array(#API => doom)).

Speaking of, I also do not understand why this access handler is registered as a service in the first place.

@disasm: D'oh! :) Please test your patches locally first. ;) I manually canceled the previous two, in order to not delay other patches in the queue. (There's no automation for that yet.)

Status:Needs review» Needs work

The last submitted patch, drupal-contact_forms-1938390-10.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.65 KB
new14.15 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Sorry sun, was trying the simplytest.me dreditor tool out (which is pretty slick). Thanks for canceling the tests for me!

Attached is a patch that successfully installed locally. It will still have the same failures from #4, but all the dependency injection should be working correctly now.

Status:Needs review» Needs work

The last submitted patch, drupal-contact_forms-1938390-13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new543 bytes
new14.17 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Attached patch adds missing use flag.

Status:Needs review» Needs work

sun: I'm not as much concerned about this class specifically security-wise. I'm concerned about global $user colliding with the expectation that the parameter will be named $user if it's a User object. In Drupal 7 we're only a naming convention away from a privilege escalation attack. With the User object naming convention, we now are begging for someone to mess up the naming two-step this issue has to cause a privilege escalation bug.

That's what I'm concerned about, and don't know how to solve without just removing global $user. (Which we all want to do, but can't do yet.)

Status:Needs work» Needs review

Why is that an issue here? Or, why can't we do what we always do? I.e., function foo(User $account)...?

Status:Needs review» Needs work

The last submitted patch, drupal-contact_forms-1938390-16.patch, failed testing.

Because then the path pattern in the route needs to be /user/{account}, which means that we have to manually specify which converter to use so that the ID gets turned into an object. That's totally doable, but "Oh yeah, even though we automatically handle any entity you have to NOT do that for users and do it manually to avoid a security risk, because we already have a global by that name" is a really crappy answer to give module developers.

I'm inclined to file a critical for global $user and force the issue. It's been a noose around our neck for years, and it's only going to tighten. We need to just solve it.

Status:Needs work» Needs review
StatusFileSize
new14.32 KB
FAILED: [[SimpleTest]]: [MySQL] 54,446 pass(es), 81 fail(s), and 34 exception(s).
[ View ]

reroll!

Status:Needs review» Needs work

The last submitted patch, drupal-contact_forms-1938390-22.patch, failed testing.

disasm, are you still working on this?

Patch also should drop _contact_personal_tab_access() helper that used now in #1853526: Reintroduce Views integration for contact.module

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.phpundefined
@@ -0,0 +1,90 @@
+class ContactPageAccess implements AccessCheckInterface {

Probably should be build on top of ContactCategoryAccessController to replace _contact_personal_tab_access()

StatusFileSize
new844 bytes
new14.47 KB
FAILED: [[SimpleTest]]: [MySQL] 57,904 pass(es), 125 fail(s), and 51 exception(s).
[ View ]

attached is a reroll with a minor typo fix.

Status:Needs work» Needs review

StatusFileSize
new1.38 KB
new13.87 KB
FAILED: [[SimpleTest]]: [MySQL] 57,872 pass(es), 126 fail(s), and 51 exception(s).
[ View ]

Just realized this has an old style bundle for registering the access check. Attached patch switches to services.yml.

Status:Needs review» Needs work
Issue tags:-needs security review, -#SprintWeekend, -WSCCI-conversion

The last submitted patch, drupal-contact_forms-1938390-28.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+needs security review, +#SprintWeekend, +WSCCI-conversion

The last submitted patch, drupal-contact_forms-1938390-28.patch, failed testing.

Needs re-roll, also there's isPersonal() method for category entity
And follow-up for #1853526: Reintroduce Views integration for contact.module

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,148 @@
+/**
+ * Throws an exception if the current user is not allowed to submit a contact form.
+ *
+ * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
+ *
+ * @see \Drupal\contact\Controller\ContactSitePage()
+ * @see \Drupal\contact\Controller\ContactPersonalPage()
+ */
+protected function contactFloodControl() {
+  $config = $this->configFactory('contact.settings');
+  $limit = $config->get('flood.limit');
+  $interval = $config->get('flood.interval');
+  if (!$this->flood->isAllowed('contact', $limit, $interval)) {
+    drupal_set_message(t("You cannot send more than %limit messages in @interval. Try again later.", array(
+      '%limit' => $limit,
+      '@interval' => format_interval($interval),
+    )), 'error');
+    throw new AccessDeniedHttpException();
+  }
+}

needs 2 spaces from left.

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.phpundefined
@@ -0,0 +1,90 @@
+    global $user;
+    $account = $request->attributes->get('user');

let's go with $user and $account but the other way round :) $account is the current active account interface and $user the user in the url.

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.phpundefined
@@ -0,0 +1,90 @@
+      return FALSE;
...
+      return FALSE;
...
+      return FALSE;
...
+      return FALSE;
...
+      return FALSE;
...
+    return user_access('access user contact forms');

Access checkers should always return static::ALLOW and static::DENY

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,148 @@
+<?php
+namespace Drupal\contact\Controller;

Missing @file

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,148 @@
+

It seems to be that this empty line is not needed.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,148 @@
+  /**
+   * Injects database service.
+   *
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface
+   *   Symfony Container Interface.

Just use {@inheritdoc}

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,148 @@
+    if (!user_access('administer contact forms')) {
...
+        if (user_access('administer contact forms')) {
...
+   */
+  public function contactPersonalPage($user) {
+    $recipient = $user;
+    global $user;

Instead we should use "AccounterInterface $account" in the method, so you get the current account directly without relying on global user. Once you have done this pass it into the user_access methods.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,148 @@
+      $categories = entity_load_multiple('contact_category');
...
+    $message = entity_create('contact_message', array(
...
+    return entity_get_form($message);
...
+    $message = entity_create('contact_message', array(
...
+    return entity_get_form($message);

entity_load_multiple|entity_create|entity_get_form could be easily replaced by an injected entity storage controller.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,148 @@
+      $default_category = config('contact.settings')->get('default_category');

This could be also injected.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,148 @@
+   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException

Try to explain when this exception is thrown.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,148 @@
+ * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException

Try to explain when this exception is thrown.

Status:Needs work» Needs review
StatusFileSize
new11.24 KB
new15.14 KB
FAILED: [[SimpleTest]]: [MySQL] 57,136 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Here is new version of the patch taking into account comments from #32 and #34.

Status:Needs review» Needs work

The last submitted patch, drupal-contact_forms-1938390-35.patch, failed testing.

Fail: 'The service "access_check.contact" has a dependency on a non-existent service "user.data".'

That test needs to enable dependency user.module.

Lots of stuff below, given that we change so much with injection and so on, let's also clean up the documentation and some weird code while we're at it.

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.phpundefined
@@ -0,0 +1,93 @@
+ * Contains Drupal\contact\Access\ContactPageAccess.

Missing leading \ in front of Drupal.

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.phpundefined
@@ -0,0 +1,93 @@
+  /**
+   * Implements AccessCheckInterface::applies().
+   */
+  public function applies(Route $route) {
+    return array_key_exists('_contact_personal_tab_access', $route->getRequirements());
+  }

appliesTo() went in, so this should now use that, see the other implementations for how that works.

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.phpundefined
@@ -0,0 +1,93 @@
+    // Anonymous users cannot have contact forms.
+    if (!$contact_account->id()) {

if ($contact_account->isAnonymous())

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.phpundefined
@@ -0,0 +1,93 @@
+    // If requested user has been blocked, do not allow users to contact them.
+    if (empty($contact_account->status)) {
+      return static::DENY;

if ($contact_account->isBlocked())

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,176 @@
+   *
+   * @param \Drupal\Core\Flood\FloodInterface $flood
+   *   Flood Control Interface.
+   *
+   * @param \Drupal\Core\Config\ConfigFactory $configFactory
+   *   Configuration Factory.
+   */
+  public function __construct(FloodInterface $flood, ConfigFactory $configFactory, EntityManager $entity_manager) {

Missing @param for entity manager.

There should be no spaces between @param's.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,176 @@
+   * @param Drupal\contact\Plugin\Core\Entity\Category $contact_category

Missing leading \

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,176 @@
+   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
+   *   Exception is thrown when user doesn't pass flood control check.
+   *
+   * @throws \Symfony\Component\HttpKernel\Exception\NotFoundHttpException
+   *   Exception is thrown when user tries to access non existing contact category
+   *   form and doesn't have permissions to set it up.

I know @dawehner requested this, but @throws, per our documentation standard, does not have a description. It's is *explicitly* documented to not have one.

I agree that it should but it's usually best to avoid discussing that in actual issues and stick to the standard.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,176 @@
+   * @see contact_menu()
+   * @see contact_site_form_submit()
+   * @ingroup forms

Not sure if it makes sense to reference the hook menu implementation here, that just defines the title and might go away completely?

I don't think the submit callback still exists, that's part of the contact message entity form controller now?

And last, this is not a form (anymore) :)

In short, drop? Or replace with useful ones, like the contact message form controller?

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,176 @@
+      $controller = $this->entityManager->getStorageController('contact_category');
+      $categories = $controller->loadMultiple();
+
+      $default_category = $this->configFactory->get('contact.settings')->get('default_category');
+      if (isset($categories[$default_category])) {
+        $contact_category = $categories[$default_category];
+      }
+      // If there are no categories, do not display the form.

Is there any reason why this is not just a load($default_category)? Loading all just to check if the default exists seems like a left-over of pre-entity API's.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,176 @@
+    }
+    $message = $this->entityManager
+      ->getStorageController('contact_message')
+      ->create(array(
+        'category' => $contact_category->id(),

Somewhere here the check about this not being the personal contact page and if so, throwing a not found exception got lost while re-rolling.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,176 @@
+   * @param $recipient

Missing type. And $recipient vs. $account.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,176 @@
+   * @see contact_menu()
+   * @see contact_personal_form_submit()
+   *
+   * @ingroup forms

Same as above.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,176 @@
+  public function contactPersonalPage(AccountInterface $account) {

This should actually type hint on UserInterface I think, as we pass in an entity object, not global $user.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,176 @@
+   * @see \Drupal\contact\Controller\ContactSitePage()
+   * @see \Drupal\contact\Controller\ContactPersonalPage()

Those classes don't exist anymore, part of the same class, so just remove it :)

Berdir asked me to comment here in IRC.

Our current docs standards say that @throws should just be followed by the name of the exception class and not a description. Presumably the Exception class name should go 90% of the way there to telling you when it would be thrown.
https://drupal.org/coding-standards/docs#throws

That said, many of the @throws tags in core now have descriptions as well, and the API module as it is now can handle this being a paragraph like @return or @param. JavaDoc supports a paragraph after the class name too. There's not probably a good reason that it should just be the class name, except that there's not much point in something like:

@throws \Whatever\Namespace\OutOfSpaceException if it's out of space

which tells you nothing that the name of the exception didn't already tell you.

So, I would be comfortable just changing the standard to match the (apparently accepted) practice in Core. In which case the standard would be to follow @throws with the name of the class and an optional description if the exception class isn't clear enough (indenting two spaces if you need to go to a new line).

Thoughts?

Ok, opened #2045181: Document and standardize how @throws should be described. to finalize and document that. Let's keep what we have now, then.

Also, $user->hasPermission() landed, so we can clean that up too...

Status:Needs work» Needs review
StatusFileSize
new7.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-contact_forms-1938390-40-interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new15.47 KB
PASSED: [[SimpleTest]]: [MySQL] 57,165 pass(es).
[ View ]

Thank you for the review! Very good catches. Here is updated patch.

Status:Needs review» Needs work

The last submitted patch, drupal-contact_forms-1938390-40-interdiff.patch, failed testing.

+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.phpundefined
@@ -0,0 +1,93 @@
+    global $user;
...
+    $contact_account = $request->attributes->get('account');
+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,167 @@
+  public function contactPersonalPage(UserInterface $account) {
...
+    global $user;

I think this is classic example of #2043781: Drupal::request()->attributes->get('account') may conflict with an account object loaded from the path. We should wait for this issue so we can remove global $user

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.phpundefined
@@ -0,0 +1,167 @@
+    if (!user_access('administer contact forms')) {
...
+        if (user_access('administer contact forms')) {

$account->hasPermission()

Status:Needs work» Needs review
StatusFileSize
new3.27 KB
new15.6 KB
PASSED: [[SimpleTest]]: [MySQL] 57,471 pass(es).
[ View ]

I was also working on this :) I had the two other calls to user_access() covered as mentioned in #42. Am looking forward to removing global $user.

+++ b/core/modules/contact/contact.routing.ymlundefined
@@ -25,3 +25,28 @@ contact_category_edit:
+  pattern: 'user/{account}/contact'
+++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.phpundefined
@@ -0,0 +1,94 @@
+    global $user;
+    // Account that we want to contact.
+    $contact_account = $request->attributes->get('account');

That is a clear problem and should wait until $request->attributes->get('account') got renamed to _account or something else.

$request->attributes->get('account') has been renamed to $request->attributes->get('_account'). Carry on! :-)

Additionally, it's now save to name the argument {user} and use $user.

StatusFileSize
new11.16 KB
new16.01 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Some cleanup here and there.

Status:Needs review» Needs work
Issue tags:-needs security review, -#SprintWeekend, -WSCCI-conversion

The last submitted patch, contact-1938390-47.patch, failed testing.

Status:Needs work» Needs review

#47: contact-1938390-47.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs security review, +#SprintWeekend, +WSCCI-conversion

The last submitted patch, contact-1938390-47.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new750 bytes
new16.03 KB
FAILED: [[SimpleTest]]: [MySQL] 57,810 pass(es), 45 fail(s), and 0 exception(s).
[ View ]

Mh another one :(

Status:Needs review» Needs work
Issue tags:-needs security review, -#SprintWeekend, -WSCCI-conversion

The last submitted patch, contact-1938390-51.patch, failed testing.

Status:Needs work» Needs review

#51: contact-1938390-51.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs security review, +#SprintWeekend, +WSCCI-conversion

The last submitted patch, contact-1938390-51.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new24.66 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
new17.34 KB

Change notice for global user https://drupal.org/node/2032447

Also access check should be different for default and configured site page so implemented contactSitePageCategory()
Building logic moved to helper method contactCategoryForm()
Used Translator for controller
Added tests
Cleaned tests

Status:Needs review» Needs work

The last submitted patch, contact-page-1938390-55.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new24.54 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
new2.45 KB

removed debug lines

Status:Needs review» Needs work
Issue tags:-needs security review, -#SprintWeekend, -WSCCI-conversion

The last submitted patch, contact-page-1938390-56.patch, failed testing.

Status:Needs work» Needs review

#57: contact-page-1938390-56.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs security review, +#SprintWeekend, +WSCCI-conversion

The last submitted patch, contact-page-1938390-56.patch, failed testing.

+1 for the interdiff posted in #57, let's get it green in the meantime.

Status:Needs work» Needs review
StatusFileSize
new25.11 KB
FAILED: [[SimpleTest]]: [MySQL] 57,532 pass(es), 30 fail(s), and 8 exception(s).
[ View ]
new1.24 KB

Merged fix for menu_item_route_access() from #2040065: Remove _account from request and use the current user service instead.
Most of failures are caused by absence $_account attribute so tabs are not rendered

Status:Needs review» Needs work

The last submitted patch, contact-page-1938390-62.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new24.32 KB
FAILED: [[SimpleTest]]: [MySQL] 57,914 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new5.97 KB

Revert useless split, should pass now most of broken tests

StatusFileSize
new14.99 KB

and interdiff of all changes

StatusFileSize
new23.1 KB
FAILED: [[SimpleTest]]: [MySQL] 57,823 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new13.83 KB
new4.29 KB

Finally should work, reverted unnecessary changes, also interdiff from 51

+++ b/core/includes/menu.incundefined
@@ -970,6 +970,7 @@ function _menu_link_translate(&$item, $translate = FALSE) {
+  $request->attributes->set('_account', Drupal::request()->attributes->get('_account'));

This hunk still required from #2040065: Remove _account from request and use the current user service instead.

StatusFileSize
new23.55 KB
FAILED: [[SimpleTest]]: [MySQL] 57,832 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new3.59 KB

Also generator needs injection

Out of scope: the displayed message and url should be changed to point to choose default category

Status:Needs review» Needs work

The last submitted patch, contact-page-1938390-67.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new25.81 KB
PASSED: [[SimpleTest]]: [MySQL] 57,963 pass(es).
[ View ]
new2.92 KB

Status:Needs review» Needs work
  1. @@ -0,0 +1,99 @@
    +    return $current_account->hasPermission('access user contact forms');

    This returns TRUE/FALSE, which is wrong. It should do:

    return hasPermission() ? static::ALLOW : static::DENY;

  2. @@ -23,10 +23,14 @@ class CategoryAccessController extends EntityAccessController {
         if ($operation == 'delete' || $operation == 'update') {
           // Do not allow delete 'personal' category used for personal contact form.
    -      return user_access('administer contact forms', $account) && $entity->id() !== 'personal';
    +      return $account->hasPermission('administer contact forms') && $entity->id() !== 'personal';
    +    }

    The comment isn't part of the patch, but since it needs to be updated anyway let's fix the grammar there. "Do not allow the 'personal' category to be deleted, as it's used for the personal contact form."

  3. @@ -0,0 +1,194 @@
    +      $default_category = $this->configFactory->get('contact.settings')->get('default_category');

    I would suggest we start injecting just the config object fro create(). During preparation for the Palantir training seminar this week we realized that it's really hard to test a class where we inject the config factory rather than the config object. If possible, just inject the config object.

  4. @@ -0,0 +1,194 @@
    +      $contact_category = $this->entityManager

    Same here. I'd suggest injecting the storage controller only from create(). That will make this line short enough to fit on one line. :-)

Status:Needs work» Needs review
StatusFileSize
new26.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,165 pass(es).
[ View ]
new2.26 KB
new4.9 KB

@Crell I does not get you suggestion about config factory but unified $contactSettings and removed config factory.

Fixed 1,2,3,4

PS: This still depends on #2062151: Create a current user service to ensure that current account is always available so contact-page-1938390-hacks.txt provides hacks (symfony & #2057607-4)

@@ -0,0 +1,194 @@
+    $this->entityManager = $entity_manager;
...
+      $container->get('plugin.manager.entity'),
...
+  public function contactSitePage(AccountInterface $_account, Category $contact_category = NULL) {
...
+      $contact_category = $this->entityManager
+        ->getStorageController('contact_category')
+        ->load($default_category);
...
+    $message = $this->entityManager
+      ->getStorageController('contact_message')
+      ->create(array(
+        'category' => $contact_category->id(),
+      ));
+
+    return $this->entityManager->getForm($message);
...
+  public function contactPersonalPage(UserInterface $user, AccountInterface $_account) {
...
+    $message = $this->entityManager
+      ->getStorageController('contact_message')
+      ->create(array(
+        'category' => 'personal',
+        'recipient' => $user->id(),
+      ));
+
+    return $this->entityManager->getForm($message);

So the entity manager still needed here

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.php
@@ -0,0 +1,193 @@
+      $contact_category = $this->entityManager
+        ->getStorageController('contact_category')
+        ->load($default_category);
+      // If there are no categories, do not display the form.

This is the part I'm suggesting you move up. In the create() method, do:

$container->get('plugin.manager.entity')->getStorageController('contact_category').

Then save just the storage controller.

Then this code block can turn into just $this->contactStorage->load($default_category);

Make sense?

  1. --- a/core/includes/menu.inc
    +++ b/core/includes/menu.inc
    +++ b/core/includes/menu.inc
    @@ -970,6 +970,7 @@ function _menu_link_translate(&$item, $translate = FALSE) {
    @@ -970,6 +970,7 @@ function _menu_link_translate(&$item, $translate = FALSE) {
    function menu_item_route_access(Route $route, $href, &$map) {
       $request = Request::create('/' . $href);
       $request->attributes->set('_system_path', $href);
    +  $request->attributes->set('_account', Drupal::request()->attributes->get('_account'));
       // Attempt to match this path to provide a fully built request to the
       // access checker.
       try {
    @@ -92,6 +92,7 @@ public function on403Html(FlattenException $exception, Request $request) {
           }
           $subrequest = Request::create('/' . $path, 'get', array('destination' => $system_path), $request->cookies->all(), array(), $request->server->all());
    +      $subrequest->attributes->set('_account', $request->attributes->get('_account'));
    ...
    @@ -165,6 +166,7 @@ public function on404Html(FlattenException $exception, Request $request) {
    @@ -165,6 +166,7 @@ public function on404Html(FlattenException $exception, Request $request) {
           //   normal system path in the site_404 variable.
           $subrequest = Request::create('/' . $path, 'get', array(), $request->cookies->all(), array(), $request->server->all());
    +      $subrequest->attributes->set('_account', $request->attributes->get('_account'));

    This contains temporary fixes from two different issues. Are we sure we want to get that in now?

  2. +++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.php
    @@ -0,0 +1,193 @@
    +    drupal_set_title($this->translator->translate('Contact @username', array('@username' => $user->getUsername())), PASS_THROUGH);

    We can finally return a title in the render array!

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.16 KB
new26.05 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

I'll let someone more knowledgable than I to decide if we want to get those temporary fixes in now or not. This patch puts the form in a render array, and adds the title there instead of using drupal_set_title(). Also rebased, and contact.pages.inc is gone now! yay!

StatusFileSize
new936 bytes
new26.03 KB
FAILED: [[SimpleTest]]: [MySQL] 53,022 pass(es), 363 fail(s), and 323 exception(s).
[ View ]

renaming render_array -> form

Status:Needs review» Needs work

The last submitted patch, drupal8.contact-module.1938390-76.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.81 KB
new26.13 KB
FAILED: [[SimpleTest]]: [MySQL] 58,233 pass(es), 159 fail(s), and 3 exception(s).
[ View ]

Addressing #72. Also, PathBasedGeneratorInterface is now UrlGeneratorInterface.

Status:Needs review» Needs work

The last submitted patch, drupal8.contact-module.1938390-78.patch, failed testing.

  1. +++ b/core/includes/menu.inc
    @@ -970,6 +970,7 @@ function _menu_link_translate(&$item, $translate = FALSE) {
    +  $request->attributes->set('_account', Drupal::request()->attributes->get('_account'));

    we can use current user service here.

  2. +++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.php
    @@ -0,0 +1,99 @@
    +    // @see contact.routing.yml

    This is not needed I think.

  3. +++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.php
    @@ -0,0 +1,99 @@
    +    if (isset($account_data) && empty($account_data)) {

    :/

  4. +++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.php
    @@ -0,0 +1,198 @@
    +class ContactPageController implements ControllerInterface {

    extends ControllerBase and remove the boiler plate code.

  5. +++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.php
    @@ -0,0 +1,198 @@
    +    drupal_set_title();
    ...
    +    $form['#title'] = $this->translator->translate('Contact @username', array('@username' => $user->getUsername()));

    If we are doing $form['#title'] why we need dst.

  6. +++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.php
    @@ -0,0 +1,198 @@
    +  /**
    +   * Throws an exception if the current user triggers flood control.
    +   *
    +   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
    +   */
    +  protected function contactFloodControl() {
    +    $limit = $this->contactSettings->get('flood.limit');
    +    $interval = $this->contactSettings->get('flood.interval');
    +    if (!$this->flood->isAllowed('contact', $limit, $interval)) {
    +      drupal_set_message($this->translator->translate('You cannot send more than %limit messages in @interval. Try again later.', array(
    +        '%limit' => $limit,
    +        '@interval' => format_interval($interval),
    +      )), 'error');
    +      throw new AccessDeniedHttpException();
    +    }
    +  }

    This has nothing to do with controller this should be in contactManger but leave it for now.

Status:Needs work» Needs review
StatusFileSize
new6.36 KB
new21.95 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Spoke with timplunkett. We definitely shouldn't touch menu.inc, anything in core, or worse symfony vendor files in a route controller patch. If there's a reason for the changes, we need a new issue for that.

Also, addressed comments in #80. This is a single patch, but has two interdiffs, one for removing the stuff that shouldn't be in it, and the other for addressing the changes in #80.

Status:Needs review» Needs work

The last submitted patch, drupal8.contact-module.1938390-81.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new30.38 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Trying this as a combined patch with #2057607: The request does not contain the _account on exception pages (403/404) and see how far we get with the tests now. If it passes, we'll postpone the issue on that.

Note to future contributors, the latest patch is in #81. DO NOT under any circumstances work off of this patch.

Status:Needs review» Needs work

The last submitted patch, drupal8.contact-module.1938390-83-combined.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.17 KB
new21.02 KB
FAILED: [[SimpleTest]]: [MySQL] 58,326 pass(es), 96 fail(s), and 18 exception(s).
[ View ]

Fixing injection in controller, and using Drupal::currentUser() in access checker.

The last submitted patch, drupal8.contact-module.1938390-85.patch, failed testing.

Status:Needs review» Needs work
Issue tags:-needs security review, -#SprintWeekend, -WSCCI-conversion

The last submitted patch, drupal8.contact-module.1938390-85.patch, failed testing.

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.

Status:Needs work» Needs review
StatusFileSize
new20.38 KB
FAILED: [[SimpleTest]]: [MySQL] 58,792 pass(es), 11 fail(s), and 0 exception(s).
[ View ]
new17.98 KB

Cleaned up and removed out of scope changes.
Added @todo for ContactPageAccess::access() #2048223: Add $account argument to AccessCheckInterface::access() method and use the current_user service

  1. +++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.php
    @@ -0,0 +1,162 @@
    +  /**
    +   * The current active user.
    +   *
    +   * @var \Drupal\Core\Session\AccountInterface
    +   */
    +  protected $currentUser;
    +
    +  /**
    ...
    +   * @param \Drupal\Core\Session\AccountInterface $current_user
    +   *   The current account service.
    +   */
    +  public function __construct(FloodInterface $flood, ConfigFactory $config, EntityManager $entity_manager, AccountInterface $current_user) {
    ...
    +    $this->currentUser = $current_user;
    ...
    +    if (!$this->currentUser->hasPermission('administer contact forms')) {

    Let's use $this->currentUser()

  2. +++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.php
    @@ -0,0 +1,162 @@
    +    $form['#title'] = $contact_category->label();
    +    return $form;
    +  }

    Should we escape that title?

Status:Needs review» Needs work

The last submitted patch, contact-page-1938390-90.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.37 KB
new19.57 KB
FAILED: [[SimpleTest]]: [MySQL] 59,135 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

Addresses comments in #91. Also, removes a bunch of injected services that aren't needed with ControllerBase.

Working on the fails in ContactLinkTest assigning to myself so that @disasm will not reroll new patch :P

Issue tags:+needs security review, +WSCCI-conversion
StatusFileSize
new3.13 KB
new22.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,793 pass(es).
[ View ]

Fixed tests.

Assigned:jibran» Unassigned

and unassigning.

+++ b/core/modules/contact/lib/Drupal/contact/Plugin/views/field/ContactLink.php
@@ -22,6 +26,53 @@
+   * The access manager for contact_personal_page route.
...
+  protected $accessManager;

I would not call that "access manager" given that there is a different class called access manager. What about the "accss checker"?

StatusFileSize
new2.1 KB
new22.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,293 pass(es).
[ View ]

access checker it is.

Status:Needs review» Reviewed & tested by the community

Nice!

StatusFileSize
new22.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,140 pass(es).
[ View ]
new4.42 KB

Issue summary:View changes

Let's work on both page callbacks at once.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new22.75 KB
PASSED: [[SimpleTest]]: [MySQL] 58,843 pass(es).
[ View ]
new1.9 KB

another merge, path changes

Status:Reviewed & tested by the community» Needs work

Patch no longer applies.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new22.78 KB
PASSED: [[SimpleTest]]: [MySQL] 59,185 pass(es).
[ View ]

Status:Reviewed & tested by the community» Needs work
Issue tags:-needs security review, -Needs reroll, -#SprintWeekend, -WSCCI-conversion

The last submitted patch, drupal8.contact-module.1938390-104.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+needs security review, +Needs reroll, +#SprintWeekend, +WSCCI-conversion

StatusFileSize
new16.85 KB
PASSED: [[SimpleTest]]: [MySQL] 59,433 pass(es).
[ View ]

StatusFileSize
new18.5 KB
FAILED: [[SimpleTest]]: [MySQL] 58,325 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.17 KB

  1. +++ b/core/modules/contact/contact.module
    @@ -86,8 +86,6 @@ function contact_menu() {
    -    'title callback' => 'entity_page_label',
    -    'title arguments' => array(1),

    This was changed in the regression issue, be sure to use the updated patch from there.

  2. +++ b/core/modules/contact/contact.module
    @@ -101,18 +99,6 @@ function contact_menu() {
    -  return \Drupal::service('access_manager')->checkNamedRoute('contact.personal_page', array('user' => $account->id()));
    +++ b/core/modules/contact/lib/Drupal/contact/Plugin/views/field/ContactLink.php
    @@ -54,7 +105,10 @@ protected function renderLink(EntityInterface $entity, ResultRow $values) {
    +    $route = $this->routeProvider->getRouteByName('contact.personal_page');
    +    if ($this->accessChecker->access($route, $request) !== StaticAccessCheckInterface::ALLOW) {

    Why isn't this using the checkNamedRoute approach?

  3. +++ b/core/modules/contact/lib/Drupal/contact/CategoryAccessController.php
    @@ -21,12 +22,18 @@ class CategoryAccessController extends EntityAccessController {
    +    $this->prepareUser($account);

    This is not needed

  4. +++ b/core/modules/contact/lib/Drupal/contact/Plugin/views/field/ContactLink.php
    @@ -54,7 +105,10 @@ protected function renderLink(EntityInterface $entity, ResultRow $values) {
    +    $request = Request::create($path);

    We have a RequestHelper instead, but see above about checkNamedRoute

Status:Needs review» Needs work

The last submitted patch, drupal8.contact-module.1938390-108.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new17.68 KB
FAILED: [[SimpleTest]]: [MySQL] 58,757 pass(es), 11 fail(s), and 0 exception(s).
[ View ]
new4.04 KB

@tim.plunkett Thanx for checkNamedRoute()

Status:Needs review» Needs work

The last submitted patch, drupal8.contact-module.1938390-110.patch, failed testing.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new16.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,810 pass(es).
[ View ]
new1.16 KB

Re-roll after #2093027: Regression: contact category titles not used for page title anymore
Also simplified code in ContactLink handler

+++ b/core/modules/contact/lib/Drupal/contact/Plugin/views/field/ContactLink.php
@@ -54,7 +92,7 @@ protected function renderLink(EntityInterface $entity, ResultRow $values) {
-    if (!_contact_personal_tab_access($entity)) {
+    if ($this->accessManager->checkNamedRoute('contact.personal_page', array('user' => $uid))) {

Thank you @tim.plunkett now i can sleep well at night. When @dawehner explained this to me in #95 I thought one line removal and 10 line addition :S.
Removing redundant tags.

Status:Needs review» Needs work

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactController.php
--- a/core/modules/contact/lib/Drupal/contact/Plugin/views/field/ContactLink.php
+++ b/core/modules/contact/lib/Drupal/contact/Plugin/views/field/ContactLink.php
+++ b/core/modules/contact/lib/Drupal/contact/Plugin/views/field/ContactLink.php
@@ -22,6 +24,42 @@
+   * Constructs a Drupal\Component\Plugin\PluginBase object.

Wrong doc.

StatusFileSize
new16.88 KB
PASSED: [[SimpleTest]]: [MySQL] 59,073 pass(es).
[ View ]
new667 bytes

fixed

Status:Needs work» Needs review

+++ b/core/modules/contact/contact.module
@@ -86,8 +86,6 @@ function contact_menu() {
-    'title callback' => 'entity_page_label',
-    'title arguments' => array(1),

Is not needed for breadcrumbs, because have no effect now

The last submitted patch, drupal8.contact-module.1938390-116.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+VDC, +#SprintWeekend, +WSCCI-conversion

Status:Needs review» Reviewed & tested by the community

I think it is ready to fly.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/contact/contact.routing.yml
@@ -42,7 +42,7 @@ contact.site_page_category:
+    _entity_access: 'contact_category.page'
+++ b/core/modules/contact/lib/Drupal/contact/CategoryAccessController.php
@@ -22,11 +23,16 @@ class CategoryAccessController extends EntityAccessController {
   public function checkAccess(EntityInterface $entity, $operation, $langcode, AccountInterface $account) {
...
+    elseif ($operation == 'page') {
+      // Do not allow access personal category via site-wide route.
+      return $account->hasPermission('access site-wide contact form') && $entity->id() !== 'personal';
     }

"page" is not a valid entity operation.

Assigned:Unassigned» alexpott
Status:Needs work» Reviewed & tested by the community

These are all entity operations in core:

'add'
'create'
'delete'
'duplicate'
'edit'
'publish'
'unpublish'
'update'
'view'

We don't limit to create/view/update/delete...

Status:Reviewed & tested by the community» Needs work

The operations that you listed are all operations (even linguistically). "page" is not an operation (not even linguistically).

Status:Needs work» Needs review
StatusFileSize
new16.88 KB
PASSED: [[SimpleTest]]: [MySQL] 59,349 pass(es).
[ View ]
new1.35 KB

Makes a lot of sense to use verb here

The operation for the contact message entity is certainly "create". That's the simple part.

Now the question is what the actual operation of the contact category is — within the scope and context of the message create operation.

Technically, you rather view the category (as in: see/view access), in order to create a message within the category.

As we're talking about entity access, "view" appears to be the proper entity operation. If you are not permitted to view the category, then you are also not allowed to create a message in that category.

Makes sense?

Yes, it makes sense too but:
1) we need a check for specific controller or route - and the global permission 'access contact forms' (view) should be extended with route-specific ID (==|!=) 'personal'
2) dx-- when we introduce ContactPageAccess classes for every single route
3) and moving more logic in access controller implementation so access controller operates with services and entities

Status:Needs review» Reviewed & tested by the community

I absolutely concede the verb part here. I was just making a point that it is not restricted to the normal operation names you would assume.

@andypost's points are all valid

Status:Reviewed & tested by the community» Needs review
  1. "contact" is definitely an improvement over "page" in terms of operation name.

  2. But please bear in mind that this is Drupal core that you are patching. Contributed and custom modules are looking at core and will interpret this code as a desired way for doing things and as a best practice.

    (Funnily, despite its stupidly simple goal and use-case, Contact module happens to encompass a range of most advanced architectural patterns.)

  3. Do we really want contrib + custom modules to nilly-willy introduce new entity operations?

    How is that going to work out in terms of security? (access permission management/control) How will I be able to control access to this new "contact" operation?

    (Given the existing operations, I'm certainly able to control "view" access to contact categories, but doing so will not encompass the "contact" operation.)

  4. Are we able to think of an entity operation name that is not limited to Contact module, and which would work for similar modules in the same space? (e.g., private messages, comment notify, etc.pp.)

@sun I think a lot about it and following your #2100397: [meta] Ensure that DX issues identified by a recent review are covered with individual issues but I see no constructive suggestions in your comments now.

Currently core have no agreement|policy about usage of EntityRenderController for config entities except block that @tim.plunkett pointed in IRC
Currently core have no way to implement access controller that depends on entity ID and the 'symfony security model' is not used

@sun Issue #1839516: Introduce entity operation providers that you filed have a patch so let's discuss entity operations in appropriate issue.

I think the scope issue is find balance between separation of the logic for each access and content controllers is done

  1. I do not understand why DX comes up as a topic here — the concern I'm raising is about entity [operations] architecture and security.

  2. I do not know what @tim.plunkett may or may not have said in IRC, because I was not in IRC when that discussion may have happened. Whenever such discussions are happening, it is best practice to post an accurate summary to the respective issue, so as to include everyone else.

  3. I do not understand why the proposed solution of using "view" as an operation name (or alternatively, finding an operation name that would work for other modules aside from Contact module) does not count as helpful/constructive.

    For completeness, I may not have fully understood #127 — what I've heard are data points that essentially suggest "'view' would be more work, but doable."

  4. The scope of this (and for this matter, any other) issue is always to ensure that a change (1) architecturally makes sense and (2) does not introduce code debt that will be hard to fix or get rid of later on.

1. security - we use mix of permissions and category ID in access controller, and entity-specific access for flood at handler level (controller)
1 & 3. operation view is not good here because 'view' of Category is 'create' for Message - so I'd prefer to have entity-specific operations same time as we have entity-specific methods

2. block module the only one that uses BlockRenderController for config entity

4. scope of this issue as part of other (meta) to make this callbacks swap-able and overridable in contrib in context of current core architecture

So I see no reason for 'make view permission work' here because then we need to put the access check into scope of route|request to make ID (personal) applies different for different routes

StatusFileSize
new16.96 KB
PASSED: [[SimpleTest]]: [MySQL] 58,946 pass(es).
[ View ]
new1.92 KB

Let's see if usage of 'view' could break

Status:Needs review» Reviewed & tested by the community

Back to RTBC as 'view' is proper verb and entity operation. I hope it also fixes @sun's concerns.

StatusFileSize
new16.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,847 pass(es).
[ View ]

re-roll

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new16.72 KB
FAILED: [[SimpleTest]]: [MySQL] 58,688 pass(es), 0 fail(s), and 8 exception(s).
[ View ]

re-roll

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal8.contact-module.1938390-137.patch, failed testing.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new17.21 KB
PASSED: [[SimpleTest]]: [MySQL] 59,266 pass(es).
[ View ]
new981 bytes

Fix broken tests

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
StatusFileSize
new17.23 KB
FAILED: [[SimpleTest]]: [MySQL] 59,931 pass(es), 11 fail(s), and 20 exception(s).
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, 141: drupal8.contact-module.1938390-141.patch, failed testing.

Status:Needs work» Needs review
Parent issue:» #1971384: [META] Convert page callbacks to controllers
Related issues:+#2105123: Add a currentUser() method to plugin base classes that need it
StatusFileSize
new1.49 KB
new17.76 KB
PASSED: [[SimpleTest]]: [MySQL] 60,286 pass(es).
[ View ]

StatusFileSize
new17.82 KB
PASSED: [[SimpleTest]]: [MySQL] 57,930 pass(es).
[ View ]

re-roll

Status:Needs review» Reviewed & tested by the community

I think this looks good now. Thanks!

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll

git ac https://drupal.org/files/issues/contact-page-1938390-144.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 18247  100 18247    0     0  11816      0  0:00:01  0:00:01 --:--:-- 18543
error: patch failed: core/modules/contact/contact.routing.yml:24
error: core/modules/contact/contact.routing.yml: patch does not apply

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new17.86 KB
PASSED: [[SimpleTest]]: [MySQL] 59,077 pass(es).
[ View ]

re-roll

Status:Reviewed & tested by the community» Fixed
Issue tags:-Needs reroll

Committed 94cba6d and pushed to 8.x. Thanks!

Removed the following use statements during commit because they are not used :)

diff --git a/core/modules/contact/contact.module b/core/modules/contact/contact.module
index bd5e60c..9b06fd9 100644
--- a/core/modules/contact/contact.module
+++ b/core/modules/contact/contact.module
@@ -5,9 +5,6 @@
  * Enables the use of personal and site-wide contact forms.
  */
-use Drupal\contact\Entity\Category;
-use Drupal\user\UserInterface;
-
/**
  * Implements hook_help().
  */

[Edit] fix link to commit hash

Status:Fixed» Closed (fixed)

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