Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#26 | 2511182-26.patch | 5.26 KB | tameeshb |
#26 | interdiff-24-26.txt | 1.51 KB | tameeshb |
#24 | content_entity_example-2511182-24.patch | 5.31 KB | navneet0693 |
#24 | interdiff.txt | 629 bytes | navneet0693 |
#21 | content_entity_example-2511182-21.patch | 5.02 KB | Nikhil Banait |
|
Comments
Comment #1
artfulrobot CreditAttribution: artfulrobot as a volunteer commentedComment #2
artfulrobot CreditAttribution: artfulrobot as a volunteer commentedComment #3
Mile23Comment #4
pp CreditAttribution: pp commentedI reviewed this patch.
I think the patch is good.
Comment #5
Mile23It 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 aNULL
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?
Comment #6
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commented@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) ?
Comment #7
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #8
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedAdded a field "role" with options "administrator" and "user" and default value to be "user".
Comment #9
Nikhil Banait CreditAttribution: Nikhil Banait as a volunteer and at QED42 commentedI 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"Comment #10
Nikhil Banait CreditAttribution: Nikhil Banait as a volunteer and at QED42 commentedAs I reviewed below two patches which works fine, hence I am changing the status of this issue.
Comment #11
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #12
Mile23Thanks for moving this forward.
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:
These docs need to all wrap at 80 chars, which is our coding standard.
Add an inline comment like this:
// Set the default value of this field to 'user'.
Comment #13
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #14
Mile23Looks pretty good. Now we need to amend the tests so they set and verify the role value.
Comment #15
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commented@Mile23, Ok. Working on it assigning to myself.
Comment #16
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #17
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #18
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 for QED42 commentedRemoved coding standard issues.
Comment #19
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 for QED42 commentedComment #20
Nikhil Banait CreditAttribution: Nikhil Banait as a volunteer and at QED42 commentedReviewed 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.
Comment #21
Nikhil Banait CreditAttribution: Nikhil Banait as a volunteer and at QED42 commentedFixed coding standard issues.
Comment #22
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 for QED42 commentedComment #23
Nikhil Banait CreditAttribution: Nikhil Banait as a volunteer and at QED42 commented@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.
Comment #24
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 for QED42 commentedIgnoring @file currently.
Removed two line spacing issues between the functions.
Comment #25
Mile23Reran the test before trying to apply the patch. Oops.
So we moved the test out from under this issue.
Comment #26
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedRerolled patch.
New test file at
./content_entity_example/tests/src/Functional/ContentEntityExampleTest.php
Please review!
Comment #27
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedYay! Patch applied! :)
Comment #28
madhavvyas CreditAttribution: madhavvyas as a volunteer commentedComment #30
Mile23Out-of-scope changes, but we'll let that slide this time. :-)
Committed and pushed. Thanks, folks!