Updated: Comment #N

Problem/Motivation

Follow up for #731724: Convert comment settings into a field to make them work with CMI and non-node entities
In comment admin paths we use comment-bundle as routing parameter.
This is of the form {entity_type}__{field_name} we then use this code:

<?php
list($entity_type, $field_name) = explode('__', $field_name, 2);
?>

To get the two relevant parts.
We could do this with dynamic routing and make these arguments available in the routing attributes.

Proposed resolution

Add an enhancer that does this.

Remaining tasks

Patch

User interface changes

None

API changes

None

Follow-up from #731724: Convert comment settings into a field to make them work with CMI and non-node entities.

Files: 
CommentFileSizeAuthor
#13 interdiff.txt1.67 KBlarowlan
#13 dynamic-routing-2098011.13.patch13.15 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 58,957 pass(es).
[ View ]
#11 interdiff.txt818 byteslarowlan
#11 dynamic-routing-2098011.11.patch13.11 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 58,947 pass(es).
[ View ]
#7 interdiff.txt4.13 KBlarowlan
#7 dynamic-routing-2098011.3.patch13.12 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 58,795 pass(es).
[ View ]
#5 interdiff.txt8.97 KBlarowlan
#5 dynamic-routing-2098011.2.patch11.5 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 58,834 pass(es).
[ View ]
#3 dynamic-routing-2098011.1.patch8.71 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,712 pass(es), 9 fail(s), and 13 exception(s).
[ View ]

Comments

andypost’s picture

Status:Postponed» Active
larowlan’s picture

Assigned:Unassigned» larowlan

Working on this too.

larowlan’s picture

Assigned:larowlan» Unassigned
Status:Active» Needs review
Issue tags:+phpunit
StatusFileSize
new8.71 KB
FAILED: [[SimpleTest]]: [MySQL] 58,712 pass(es), 9 fail(s), and 13 exception(s).
[ View ]

Here tis, also adds new unit-test for the enhancer

Status:Needs review» Needs work

The last submitted patch, dynamic-routing-2098011.1.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new11.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,834 pass(es).
[ View ]
new8.97 KB

Should fix tests.

jibran’s picture

Status:Needs review» Needs work

Nice clean up. It is almost RTBC just minor things.

  1. +++ b/core/modules/comment/lib/Drupal/comment/Routing/CommentBundleEnhancer.php
    @@ -0,0 +1,51 @@
    +  public function __construct(EntityManager $entity_manager) {

    @param missing.

  2. +++ b/core/modules/comment/tests/Drupal/comment/Tests/Routing/CommentBundleEnhancerTest.php
    @@ -0,0 +1,60 @@
    +    $request = new Request();
    +    $defaults = array('bundle' => 'node__comment');
    +    $new_defaults = $route_enhancer->enhance($defaults, $request);
    ...
    +    $request = new Request();
    +    $defaults = array('bundle' => 'node__field_foobar');
    +    $new_defaults = $route_enhancer->enhance($defaults, $request);

    This seems like a good use of dataProvider.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new13.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,795 pass(es).
[ View ]
new4.13 KB

Fixes #6 and merges HEAD

Status:Needs review» Needs work
Issue tags:-phpunit

The last submitted patch, dynamic-routing-2098011.3.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
Issue tags:+phpunit

#7: dynamic-routing-2098011.3.patch queued for re-testing.

jibran’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+API clean-up

Thank you @larowlan for the fixes :). Now we have more test ;). Clean code. I think it is api clean up.

  1. +++ b/core/modules/comment/tests/Drupal/comment/Tests/Routing/CommentBundleEnhancerTest.php
    @@ -0,0 +1,101 @@
    +  /**
    +   * Data provider for testEnhancer().
    +   *
    +   * @see testEnhancer()
    +   *
    +   * @return array
    +   *   An array of arrays containing strings:
    +   *     - The bundle name.
    +   *     - The commented entity type.
    +   *     - The field name.
    +   */
    +  public function providerTestEnhancer() {
    +    return array(
    +      array(
    +        'node__comment',
    +        'comment',
    +        'node',
    +      ),
    +      array(
    +        'node__comment_forum__schnitzel',
    +        'comment_forum__schnitzel',
    +        'node',
    +      ),
    +      array(
    +        'node__field_foobar',
    +        FALSE,
    +        FALSE
    +      ),
    +    );
    +  }

    Wow this is called beauty :D.

  2. +++ b/core/modules/comment/tests/Drupal/comment/Tests/Routing/CommentBundleEnhancerTest.php
    @@ -0,0 +1,101 @@
    +        // Test two sets of __.CommentManager

    CommentManager is a typo :P. Please can we fix it on commit :$.

larowlan’s picture

StatusFileSize
new13.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,947 pass(es).
[ View ]
new818 bytes

Fixes typo

dawehner’s picture

  1. +++ b/core/modules/comment/lib/Drupal/comment/Routing/CommentBundleEnhancer.php
    @@ -0,0 +1,54 @@
    +    $bundles = $this->entityManager->getBundleInfo('comment');

    Can we move it to the if() so that the bundles are not loaded on every routing request.

  2. +++ b/core/modules/comment/lib/Drupal/comment/Routing/CommentBundleEnhancer.php
    @@ -0,0 +1,54 @@
    +    if (isset($defaults['bundle']) && isset($bundles[$defaults['bundle']])) {

    Just looking at this line I am wondering whether this is maybe too generic. Isn't there quite a high chance that someone will put also 'bundle' into the route and it will magically adds these values even not needed?

  3. +++ b/core/modules/comment/tests/Drupal/comment/Tests/Routing/CommentBundleEnhancerTest.php
    @@ -0,0 +1,101 @@
    +/**
    + * Tests the comment bundle enhancer route enhancer.
    + *
    + * @see \Drupal\comment\Routing\CommentBundleEnhancer
    + */

    We are adding @group Drupal these days. I think it just makes sense.

larowlan’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new13.15 KB
PASSED: [[SimpleTest]]: [MySQL] 58,957 pass(es).
[ View ]
new1.67 KB
  1. Yes, good idea - done
  2. No it will only do it when the bundle is a comment bundle see isset($bundles[$defaults['bundle']])
  3. Done, also added @group Routing as a few have that too
dawehner’s picture

Status:Needs review» Reviewed & tested by the community

No it will only do it when the bundle is a comment bundle see isset($bundles[$defaults['bundle']])

At least this does not change the bundles variable.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 8caad96 and pushed to 8.x. Thanks!

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