The current Content class includes a 'default_value' => '' array key to setSettings() which implies that this does something, but it does not. Or does not any more.

This patch following uses the setDefaultValue() method to properly indicate how a default value should be set.

While I suspect the code does nothing anyway (it sets ZLS for the default, which is going to be the default anyway, at least in terms of the form) it seems important for example code to show the right way of doing things.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

artfulrobot’s picture

artfulrobot’s picture

Issue summary: View changes
Mile23’s picture

Status: Active » Needs review
pp’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed this patch.

  1. I installed d8 with standard profile
  2. Switched on Content Entity Example module
  3. Added a Contact without Name and First Name value
  4. Checked the log messages (no errors)
  5. Deleted the Contact, and uninstalled the module.
  6. Apply a patch
  7. Repeat 2-4
  8. No error found.

I think the patch is good.

Mile23’s picture

Status: Reviewed & tested by the community » Needs work

It would be great to have a field that needs an actual default value, rather than (wrongly) demonstrating that a field with no default should have its default set. According to the docs, setDefaultValue() should take a NULL or empty array to specify no default. https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Field%21B...

I think in this case, we shouldn't set a default value for any of the existing fields. We should add a field called 'role,' with a selection of 'administrator' and 'user', which then lets us set a default value of 'user'. Good idea?

navneet0693’s picture

@Mile23,

So should we remove the following fields from code : name and first_name and add a new field name "role" with selection options "user" and "administrator" ? Or, should we keep both fields and add a new one (role) ?

navneet0693’s picture

Assigned: artfulrobot » navneet0693
navneet0693’s picture

Added a field "role" with options "administrator" and "user" and default value to be "user".

Nikhil Banait’s picture

I reviewed both patches, works fine. No Issues.

So here I am creating new patch which is basically combination of above both patches, as setDefaultValue() is required instead of 'default_value' => ''. Also fixed coding standard issues in patch "content_entity_example-2511182-8.patch"

Nikhil Banait’s picture

Status: Needs review » Reviewed & tested by the community

As I reviewed below two patches which works fine, hence I am changing the status of this issue.

  1. properly-set-default-value-2511182-0.patch
  2. content_entity_example-2511182-8.patch
navneet0693’s picture

Assigned: navneet0693 » Unassigned
Mile23’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for moving this forward.

  1. +++ b/content_entity_example/src/Entity/Contact.php
    @@ -222,10 +222,10 @@ class Contact extends ContentEntityBase implements ContactInterface {
    +      ->setDefaultValue('')
    
    @@ -242,10 +242,10 @@ class Contact extends ContentEntityBase implements ContactInterface {
    +      ->setDefaultValue('')
    

    Like I mentioned in #5, the way you specify no default is to pass in NULL or empty array: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Field%21B...

    We also want to explain this in inline comments.

    So those should be:

      // Set no default value.
      ->setDefaultValue(NULL)
    
  2. +++ b/content_entity_example/src/Entity/Contact.php
    @@ -310,6 +310,33 @@ class Contact extends ContentEntityBase implements ContactInterface {
    +    // Role field for the contact.
    +    // ListTextType with a drop down menu widget.
    +    // The values shown in the menu are 'administrator' and 'user'.
    +    // In the view the field content is shown as string.
    +    // In the form the choices are presented as options list.
    

    These docs need to all wrap at 80 chars, which is our coding standard.

  3. +++ b/content_entity_example/src/Entity/Contact.php
    @@ -310,6 +310,33 @@ class Contact extends ContentEntityBase implements ContactInterface {
    +      ->setDefaultValue('user')
    

    Add an inline comment like this:

    // Set the default value of this field to 'user'.

navneet0693’s picture

Mile23’s picture

Status: Needs review » Needs work

Looks pretty good. Now we need to amend the tests so they set and verify the role value.

navneet0693’s picture

@Mile23, Ok. Working on it assigning to myself.

navneet0693’s picture

Assigned: Unassigned » navneet0693
navneet0693’s picture

Assigned: navneet0693 » Unassigned
Status: Needs work » Needs review
FileSize
5.1 KB
3.38 KB
navneet0693’s picture

navneet0693’s picture

Nikhil Banait’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed patch and did testing as well for "Drupal\content_entity_example\Tests\ContentEntityExampleTest". Worked fine.
Results:
241 passes, 0 fails, 0 exceptions, 60 debug messages

Fixing spacing coding standard issue in next comment.

Nikhil Banait’s picture

Fixed coding standard issues.

navneet0693’s picture

Status: Reviewed & tested by the community » Needs review
Nikhil Banait’s picture

@Mile23 Do we need to remove @file documentation comment in this issue or should we create new issue for this one?
Because files that contain a namespaced class/interface/trait, whose file name is the class name with a .php extension SHOULD NOT have a @file documentation block. https://www.drupal.org/coding-standards/docs#file

All other example sub modules containing @file documentation block, so I think its better to create new issue.

Please let me know your take on this.

navneet0693’s picture

Ignoring @file currently.
Removed two line spacing issues between the functions.

Mile23’s picture

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

Reran the test before trying to apply the patch. Oops.

$ git apply content_entity_example-2511182-24.patch 
error: content_entity_example/src/Tests/ContentEntityExampleTest.php: No such file or directory

So we moved the test out from under this issue.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
5.26 KB

Rerolled patch.
New test file at
./content_entity_example/tests/src/Functional/ContentEntityExampleTest.php
Please review!

tameeshb’s picture

Yay! Patch applied! :)

madhavvyas’s picture

Issue tags: -Needs reroll

  • Mile23 committed 1ce360e on 8.x-1.x authored by tameeshb
    Issue #2511182 by navneet0693, Nikhil Banait, tameeshb, artfulrobot, pp...
Mile23’s picture

Status: Needs review » Fixed
+++ b/content_entity_example/src/Entity/Controller/ContactListBuilder.php
@@ -23,7 +23,6 @@ class ContactListBuilder extends EntityListBuilder {
-

@@ -50,7 +49,6 @@ class ContactListBuilder extends EntityListBuilder {
-

Out-of-scope changes, but we'll let that slide this time. :-)

Committed and pushed. Thanks, folks!

Status: Fixed » Closed (fixed)

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