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.

CommentFileSizeAuthor
#147 contact-page-1938390-147.patch17.86 KBandypost
#144 contact-page-1938390-144.patch17.82 KBandypost
#143 drupal8.contact-module.1938390-143.patch17.76 KBandypost
#143 interdiff.txt1.49 KBandypost
#141 drupal8.contact-module.1938390-141.patch17.23 KBpfrenssen
#139 interdiff.txt981 bytesandypost
#139 drupal8.contact-module.1938390-139.patch17.21 KBandypost
#137 drupal8.contact-module.1938390-137.patch16.72 KBandypost
#135 drupal8.contact-module.1938390-135.patch16.89 KBandypost
#133 interdiff.txt1.92 KBandypost
#133 drupal8.contact-module.1938390-133.patch16.96 KBandypost
#125 interdiff.txt1.35 KBandypost
#125 drupal8.contact-module.1938390-125.patch16.88 KBandypost
#116 interdiff.txt667 bytesandypost
#116 drupal8.contact-module.1938390-116.patch16.88 KBandypost
#113 interdiff.txt1.16 KBandypost
#113 drupal8.contact-module.1938390-112.patch16.9 KBandypost
#111 interdiff.txt4.04 KBandypost
#111 drupal8.contact-module.1938390-110.patch17.68 KBandypost
#108 interdiff.txt2.17 KBandypost
#108 drupal8.contact-module.1938390-108.patch18.5 KBandypost
#107 drupal8.contact-module.1938390-107.patch16.85 KBandypost
#104 drupal8.contact-module.1938390-104.patch22.78 KBandypost
#102 interdiff.txt1.9 KBandypost
#102 drupal8.contact-module.1938390-102.patch22.75 KBandypost
#100 interdiff.txt4.42 KBandypost
#100 drupal8.contact-module.1938390-100.patch22.7 KBandypost
#98 drupal8.contact-module.1938390-98.patch22.7 KBjibran
#98 interdiff.txt2.1 KBjibran
#95 drupal8.contact-module.1938390-95.patch22.7 KBjibran
#95 interdiff.txt3.13 KBjibran
#93 drupal8.contact-module.1938390-93.patch19.57 KBdisasm
#93 interdiff.txt5.37 KBdisasm
#90 interdiff.txt17.98 KBandypost
#90 contact-page-1938390-90.patch20.38 KBandypost
#85 drupal8.contact-module.1938390-85.patch21.02 KBdisasm
#85 interdiff.txt5.17 KBdisasm
#83 drupal8.contact-module.1938390-83-combined.patch30.38 KBdisasm
#81 drupal8.contact-module.1938390-81.patch21.95 KBdisasm
#81 interdiff-changes-80.txt6.36 KBdisasm
#78 drupal8.contact-module.1938390-78.patch26.13 KBdisasm
#78 interdiff.txt3.81 KBdisasm
#76 drupal8.contact-module.1938390-76.patch26.03 KBdisasm
#76 interdiff.txt936 bytesdisasm
#75 drupal8.contact-module.1938390-75.patch26.05 KBdisasm
#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
#69 interdiff.txt2.92 KBandypost
#69 contact-page-1938390-69.patch25.81 KBandypost
#67 interdiff.txt3.59 KBandypost
#67 contact-page-1938390-67.patch23.55 KBandypost
#66 interdiff.txt4.29 KBandypost
#66 interdiff-51vs66.txt13.83 KBandypost
#66 contact-page-1938390-66.patch23.1 KBandypost
#65 interdiff-51vs63.txt14.99 KBandypost
#64 interdiff.txt5.97 KBandypost
#64 contact-page-1938390-63.patch24.32 KBandypost
#62 interdiff.txt1.24 KBandypost
#62 contact-page-1938390-62.patch25.11 KBandypost
#57 interdiff.txt2.45 KBandypost
#57 contact-page-1938390-56.patch24.54 KBandypost
#55 interdiff.txt17.34 KBandypost
#55 contact-page-1938390-55.patch24.66 KBandypost
#51 contact-1938390-51.patch16.03 KBdawehner
#51 interdiff.txt750 bytesdawehner
#47 contact-1938390-47.patch16.01 KBdawehner
#47 interdiff.txt11.16 KBdawehner
#43 drupal-contact_forms-1938390-43.patch15.6 KBpfrenssen
#43 interdiff.txt3.27 KBpfrenssen
#40 drupal-contact_forms-1938390-40.patch15.47 KBygerasimov
#40 drupal-contact_forms-1938390-40-interdiff.patch7.62 KBygerasimov
#35 drupal-contact_forms-1938390-35.patch15.14 KBygerasimov
#35 drupal-contact_forms-1938390-35-interdiff.txt11.24 KBygerasimov
#28 drupal-contact_forms-1938390-28.patch13.87 KBdisasm
#28 interdiff.txt1.38 KBdisasm
#26 drupal-contact_forms-1938390-26.patch14.47 KBdisasm
#26 interdiff.txt844 bytesdisasm
#22 drupal-contact_forms-1938390-22.patch14.32 KBdisasm
#16 drupal-contact_forms-1938390-16.patch14.17 KBdisasm
#16 interdiff.txt543 bytesdisasm
#14 drupal-contact_forms-1938390-13.patch14.15 KBdisasm
#14 interdiff.txt4.65 KBdisasm
#10 drupal-contact_forms-1938390-10.patch13.94 KBdisasm
#10 interdiff.txt687 bytesdisasm
#9 drupal-contact_forms-1938390-9.patch13.93 KBdisasm
#9 interdiff.txt681 bytesdisasm
#7 drupal-contact_forms-1938390-7.patch13.93 KBdisasm
#7 interdiff.txt5.72 KBdisasm
#4 drupal-contact_forms-1938390-4.patch13.04 KBdisasm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Title: Convert contact_site_page to a new-style Controller » Convert contact_site_page and contact_person_page to a new-style Controller
Crell’s picture

disasm’s picture

Assigned: Unassigned » disasm
disasm’s picture

Status: Active » Needs review
FileSize
13.04 KB

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.

Crell’s picture

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

disasm’s picture

Status: Needs work » Needs review
Issue tags: -Needs security review
FileSize
5.72 KB
13.93 KB

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.

disasm’s picture

Issue tags: +Needs security review
disasm’s picture

attached fixes argument on ContactBundle

disasm’s picture

Third times a charm I hope!

sun’s picture

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.

sun’s picture

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

disasm’s picture

Status: Needs work » Needs review
FileSize
4.65 KB
14.15 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
543 bytes
14.17 KB

Attached patch adds missing use flag.

Crell’s picture

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

Crell’s picture

Status: Needs work » Needs review
sun’s picture

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.

Crell’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
14.32 KB

reroll!

Status: Needs review » Needs work

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

kim.pepper’s picture

disasm, are you still working on this?

andypost’s picture

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()

disasm’s picture

attached is a reroll with a minor typo fix.

disasm’s picture

Status: Needs work » Needs review
disasm’s picture

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, -SprintWeekend2013, -WSCCI-conversion

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

disasm’s picture

Status: Needs work » Needs review

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

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

andypost’s picture

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.

andypost’s picture

dawehner’s picture

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

ygerasimov’s picture

Status: Needs work » Needs review
FileSize
11.24 KB
15.14 KB

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.

Berdir’s picture

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 :)

jhodgdon’s picture

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?

Berdir’s picture

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

ygerasimov’s picture

Status: Needs work » Needs review
FileSize
7.62 KB
15.47 KB

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.

jibran’s picture

+++ 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()

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
3.27 KB
15.6 KB

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.

dawehner’s picture

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

Crell’s picture

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

Berdir’s picture

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

dawehner’s picture

FileSize
11.16 KB
16.01 KB

Some cleanup here and there.

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

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

disasm’s picture

Status: Needs work » Needs review

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

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

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
750 bytes
16.03 KB

Mh another one :(

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

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

dawehner’s picture

Status: Needs work » Needs review

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

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

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

andypost’s picture

Status: Needs work » Needs review
FileSize
24.66 KB
17.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.

andypost’s picture

Status: Needs work » Needs review
FileSize
24.54 KB
2.45 KB

removed debug lines

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

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

andypost’s picture

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, +SprintWeekend2013, +WSCCI-conversion

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

dawehner’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
25.11 KB
1.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.

andypost’s picture

Status: Needs work » Needs review
FileSize
24.32 KB
5.97 KB

Revert useless split, should pass now most of broken tests

andypost’s picture

FileSize
14.99 KB

and interdiff of all changes

andypost’s picture

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.

andypost’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
25.81 KB
2.92 KB
Crell’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
26.02 KB
2.26 KB
4.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

Crell’s picture

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

dawehner’s picture

  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!

Crell’s picture

Status: Needs review » Needs work
disasm’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
26.05 KB

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!

disasm’s picture

renaming render_array -> form

Status: Needs review » Needs work

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

disasm’s picture

Status: Needs work » Needs review
FileSize
3.81 KB
26.13 KB

Addressing #72. Also, PathBasedGeneratorInterface is now UrlGeneratorInterface.

Status: Needs review » Needs work

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

jibran’s picture

  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.

disasm’s picture

Status: Needs work » Needs review
FileSize
6.36 KB
21.95 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
30.38 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
5.17 KB
21.02 KB

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, -SprintWeekend2013, -WSCCI-conversion

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

andypost’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
20.38 KB
17.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

dawehner’s picture

  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.

disasm’s picture

Status: Needs work » Needs review
FileSize
5.37 KB
19.57 KB

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

jibran’s picture

Assigned: disasm » jibran
Issue tags: -Needs security review, -WSCCI-conversion

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

jibran’s picture

jibran’s picture

Assigned: jibran » Unassigned

and unassigning.

dawehner’s picture

+++ 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"?

jibran’s picture

access checker it is.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice!

andypost’s picture

andypost’s picture

Issue summary: View changes

Let's work on both page callbacks at once.

webchick’s picture

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

Patch no longer applies.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
22.75 KB
1.9 KB

another merge, path changes

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Patch no longer applies.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
22.78 KB

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

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

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Needs security review, +Needs reroll, +SprintWeekend2013, +WSCCI-conversion
andypost’s picture

andypost’s picture

tim.plunkett’s picture

  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.

andypost’s picture

Status: Needs work » Needs review
FileSize
17.68 KB
4.04 KB

@tim.plunkett Thanx for checkNamedRoute()

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.9 KB
1.16 KB

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

jibran’s picture

Issue tags: -Needs security review +VDC
+++ 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.

dawehner’s picture

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.

andypost’s picture

andypost’s picture

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.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +VDC, +SprintWeekend2013, +WSCCI-conversion
jibran’s picture

Status: Needs review » Reviewed & tested by the community

I think it is ready to fly.

tim.plunkett’s picture

sun’s picture

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.

tim.plunkett’s picture

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

sun’s picture

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

andypost’s picture

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

Makes a lot of sense to use verb here

sun’s picture

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?

andypost’s picture

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

tim.plunkett’s picture

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

sun’s picture

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

andypost’s picture

@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

sun’s picture

  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.

andypost’s picture

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

andypost’s picture

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

jibran’s picture

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.

andypost’s picture

re-roll

alexpott’s picture

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

Patch no longer applies.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
16.72 KB

re-roll

Status: Reviewed & tested by the community » Needs work

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

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
17.21 KB
981 bytes

Fix broken tests

kim.pepper’s picture

kim.pepper’s picture

Issue summary: View changes

Updated issue summary.

pfrenssen’s picture

Issue summary: View changes
FileSize
17.23 KB

Rerolled.

Status: Needs review » Needs work

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

andypost’s picture

andypost’s picture

re-roll

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I think this looks good now. Thanks!

alexpott’s picture

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
andypost’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
17.86 KB

re-roll

alexpott’s picture

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.