Expand ContactForm with methods

Going to work on this.

@kgoel Did you have any questions or intermediate work to post?

@YesCT I am going to look at other similar patches and work on this today.

Working on this now.

#5: expand-category-methods-2030591-5.patch queued for re-testing.

I have tried running test locally to see if patch passes but I ran into some error while running test - Uncaught PHP Exception Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: "The service "access_check.contact_personal" has a dependency on a non-existent service "user.data"."

I googled the error and found the other issue is also running into the same error https://drupal.org/node/1938390 and looks like this issue depend on https://drupal.org/node/2048223.

@kgoel: The two issues you are referring to are fixed #1938390: Convert contact_site_page and contact_person_page to a new-style Controller and #2048223: Add $account argument to AccessCheckInterface::access() method and use the current_user service.

Thanks Daffie!

12: expand-category-methods-2030591-11.patch queued for re-testing.

Here is updated patch, it still need some work.

Forgot to change the status.

Added wrong patch, adding correct patch.

@sharique, your patch is not able to set the properties like label/recipients/weight/reply for the contact form categories.

Uploading another patch with this fixed.

Adding back the test class from patch in comment#11.

Added missing test file provided by kgoel, to last patch.

When I run tests for CategoryMethodsTest it gives following error, can somebody help me understand this error.

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: The service "access_check.contact_personal" has a dependency on a non-existent service "user.data". in Symfony\Component\DependencyInjection\Compiler\CheckExceptionOnInvalidReferenceBehaviorPass->processReferences() (line 59 of /home/sharique/Projects/drupal.org/d8/drupal/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php).

Looking at CategoryMethodsTest I see nothing defines $this->category
I think you needed an entity_create() before you start asserting things

Updated patch. Fixed failing tests.

Re roll as CategoryListController is now CategoryListBuilder

1. +++ b/core/modules/contact/lib/Drupal/contact/Entity/Category.php
   @@ -63,18 +65,77 @@ class Category extends ConfigEntityBase implements CategoryInterface {
   -  public $reply = '';
   +  public $reply;
   ...
   -  public $weight = 0;
   +  public $weight;
   

   any reason to drop default values?

2. +++ b/core/modules/contact/lib/Drupal/contact/Tests/CategoryMethodsTest.php
   @@ -0,0 +1,90 @@
   +class CategoryMethodsTest extends WebTestBase {
   

   makes sense as unittest

Should the function setLabel be:

  public function setLabel($label) {
    $this->set('label', $label);
    return $this;
  }

Instead of:

  public function setLabel($label) {
    return $this->set('label', $label);
  }

Just a reroll...

1. +++ b/core/modules/contact/src/Entity/Category.php
   @@ -50,7 +51,7 @@ class Category extends ConfigEntityBundleBase implements CategoryInterface {
   +   * The human-readable label of this catagory.
   

   Bit of a nitpick, sorry but got a typo here.

2. +++ b/core/modules/contact/src/Entity/Category.php
   @@ -64,17 +65,76 @@ class Category extends ConfigEntityBundleBase implements CategoryInterface {
   +   * An auto-reply message
   

   Missing full stop?

3. +++ b/core/modules/contact/src/Entity/Category.php
   @@ -64,17 +65,76 @@ class Category extends ConfigEntityBundleBase implements CategoryInterface {
   +  public $reply;
   

   As andypost pointed out, any reason to remove the defaults?

Just a couple minor things.

oh also +1 for andypost's comment #30 - "makes sense as unittest"

Did minor correction mention in #35 and #36. Please review updated patch.

Just a mini review

This patch introduces a new test CategoryMethodsTest and the getInfo() function has been removed from core.

#697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)

There is no drama the group information needs to be moved into annotations :-

I will just include the following snippet, for when the patch is back in a healthy state.

 /**
  * Tests get and set methods on the category interface.
+ *
+ * @group Contact
  */
 class CategoryMethodsTest extends DrupalUnitTestBase {
 
@@ -28,14 +30,6 @@ class CategoryMethodsTest extends DrupalUnitTestBase {
    */
   private $category;
 
-  public static function getInfo() {
-    return array(
-      'name' => 'Category get and set methods',
-      'description' => 'Test get and set methods on category interface',
-      'group' => 'Contact',
-    );
-  }
-
   protected function setUp() {
     parent::setUp();
     //$this->installConfig(array('contact'));

Reroll.

Summary of test failure, problems resolving user.data, a perfectly well defined service.

Solution: CategoryMethodsTest was not enabling the user module, so could not find the service.

@martin107 There's no reason to call save() to make sure that set/get works

Let's see if we can make the properties protected to make sure they are not accessed directly
Converted test to unit-test, cleaned-up doc-blocks

Fixed up a few things.

Awesome, +1 to rtbc!
@berdir anything left here?

Looks fine to me.

1. +++ b/core/modules/contact/src/CategoryInterface.php
   @@ -14,4 +14,76 @@
   +  /**
   +   * Returns the category label.
   +   *
   +   * @return string
   +   *   The category label.
   +   */
   +  public function getLabel();
   
   +++ b/core/modules/contact/src/Entity/Category.php
   @@ -47,34 +48,93 @@ class Category extends ConfigEntityBundleBase implements CategoryInterface {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getLabel() {
   +    return $this->get('label');
   +  }
   

   Is this really needed - we have a label() method already?

2. +++ b/core/modules/contact/src/CategoryInterface.php
   @@ -14,4 +14,76 @@
   +  /**
   +   * Sets the category label.
   +   *
   +   * @param string $label
   +   *   The desired label.
   +   *
   +   * @return $this
   +   */
   +  public function setLabel($label);
   ...
   +  /**
   +   * Sets list of recipient e-mail addresses.
   +   *
   +   * @param array $recipients
   +   *   The desired list of e-mail addresses of this category.
   +   *
   +   * @return $this
   +   */
   +  public function setRecipients($recipients);
   ...
   +  /**
   +   * Sets an auto-reply message to send to the message author.
   +   *
   +   * @param string $reply
   +   *   The desired reply.
   +   *
   +   * @return $this
   +   */
   +  public function setReply($reply);
   ...
   +  /**
   +   * Sets the weight.
   +   *
   +   * @param int $weight
   +   *   The desired weight.
   +   *
   +   * @return $this
   +   */
   +  public function setWeight($weight);
   
   +++ b/core/modules/contact/src/Entity/Category.php
   @@ -47,34 +48,93 @@ class Category extends ConfigEntityBundleBase implements CategoryInterface {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setLabel($label) {
   +    return $this->set('label', $label);
   +  }
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setRecipients($recipients) {
   +    $this->set('recipients', $recipients);
   +    return $this;
   +  }
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setReply($reply) {
   +    $this->set('reply', $reply);
   +    return $this;
   +  }
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setWeight($weight) {
   +    $this->set('weight', $weight);
   +    return $this;
   +  }
   

   These setter's are never used (outside of a test added by this patch) - should we really be adding them?

@alexpott: Because core does not use the setter functions does not mean that contrib will not use them either. Unless you have a good argument why contrib not not use them.

+1 to use label() over getLabel()
Happy to keep the setters - no harm in having them, might save a contrib module using reflection :)

Removed getLabel() and replaced it with label()

Triggering tests

note #2285083: Rename contact category to contact form is rtbc which will require a major re-roll here

Reroll, from last good (#47)

using label() not getLabel()

review from #50 still needs addressing.

Fixed up one of the unit tests.

Other errors persist.

Fixed up errors common to many tests.

So a summary of the work needed from #50 combined with this quote from #52

Happy to keep the setters - no harm in having them, might save a contrib module using reflection :)

That boils down to just remove the set/getLabel stuff.

So snip snip, the patch is smaller, there is no outstanding work to be done on this issue.

Patch mostly wrong because does not take changes from commited #2285083: Rename contact category to contact form
Also about label there's #2199917: Implement EntityLabeledInterface

1. +++ b/core/modules/contact/src/CategoryForm.php
   @@ -0,0 +1,126 @@
   + * Base form for category edit forms.
   ...
   +class CategoryForm extends EntityForm {
   

   There's ContactFormEditForm now

2. +++ b/core/modules/contact/src/CategoryInterface.php
   @@ -0,0 +1,89 @@
   +interface CategoryInterface extends ConfigEntityInterface {
   
   +++ b/core/modules/contact/src/Entity/ContactForm.php
   @@ -11,6 +11,8 @@
   +use Drupal\contact\CategoryInterface;
   
   +++ b/core/modules/contact/tests/src/ContactFormTest.php
   @@ -0,0 +1,73 @@
   + * @coversDefaultClass \Drupal\contact\Entity\Category
   ...
   +    $this->category = new ContactForm(array(), 'contact_category');
   

   Here's ContactFormInterface to extend and ContactForm entity

Just a reroll, that removes CategoryForm and CategoryInterface.
Also not sure if we should add setters, in most issues from this META @alexpott asked to remove setters unused in core. I left setters for now, but for consistency setters have to be removed here also?

Fix the whitespace issue and just a simple reroll. I think the additional setters are fine here. This is ready to go so RTBC.

#2325801: Abstract contact module mail delivery out of form into a service is postpone on this.

I also don't have a problem with the setters specific to Contact module's stuff (and looking at a few others such as Menu.php and NodeType.php we seem to do this elsewhere), but setLabel() is problematic because it's a property that's found on all entities, and therefore if we're going to have a setter for it, it should go on a base class somewhere.

So can we get a quick re-roll without that setter? Otherwise this looked fine to me, and looks like Alex's other feedback has been addressed.

Ok

Hm, I don't agree with that.

because it's a property that's found on all entities

This is not correct. All entities have a label() *method*, that gives unified access to a label of the entity, which might map to whatever property/field an entity uses for that. For nodes, it's the title (and it therefore has setTitle()), for terms, it's the name (so it has setName()) and so on.

However, for contact forms, the label property is actually called label, so it makes sense to have a corresponding setter for it, just like we have for all other properties.

Berdir and I talked about his concerns in IRC. Basically, while all entities do have a label() method courtesy of EntityInterface, it's not always consistent what this returns. Some entities, such as entity displays, just return NULL and have no label at all. Others, such as User, return the value of the username. Therefore, it's often unclear what setLabel() would actually do in those cases, so throwing it on the base class doesn't really make sense. In this case, however, label is a legit property that you may want to set (though core doesn't seem to have such a case, given the patch only adds the methods).

I'm not super comfortable with only adding it to one arbitrary config entity though and not others that have a similar "profile," because it creates a DX mis-impression (I was not able to find any other examples of config entities that have a setLabel() function in my arbitrary clicking around, so this would introduce inconsistency). So I think I'm still more comfortable going with #72 over #69 at this point, though I'd be very cool w/ a follow-up to discuss how to handle label setters more generally.

With that, committed and pushed to 8.x. Thanks! Also thanks for answering my stupid questions, Berdir. :)