These two methods in FieldItemInterface look like they're totally separate, but other parts of core assume that the setting names are distinct.
For example in BaseFieldDefinition the two arrays are merged:
$default_settings = $field_type_manager->getDefaultStorageSettings($type) + $field_type_manager->getDefaultFieldSettings($type);
Document in both methods that setting names must be different from the ones returned by the other method.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | Screenshot 2024-05-17 at 2.48.48 PM.png | 279.81 KB | isalmanhaider |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
isalmanhaider commentedExplanation:
In Drupal 11, the FieldItemInterface includes two methods: defaultFieldSettings() and defaultStorageSettings(). These methods define default settings for field items at different levels:
- defaultFieldSettings(): Specifies default settings at the field level.
- defaultStorageSettings(): Specifies default settings at the storage level.
Issue:
The methods are expected to return settings with unique names. This is crucial because other parts of the core, like BaseFieldDefinition, merge these arrays. Overlapping names can lead to conflicts or unexpected behavior.
Solution:
Document in both methods that setting names must be distinct to avoid such issues.
Comment #3
karimb commentedWe are going to take this issue as part of the Symetris contribution workshop.
Comment #4
quietone commentedFixes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.
Comment #5
rodrigoaguileraThe Drupal Contribution Mentoring team is triaging issues for DrupalCon Barcelona 2024, and we are reserving this issue for Mentored Contribution during the event.
After September 27, 2024, this issue returns to being open to all. Thanks!
I guess this issue never made it into the workshop. We can work on it in Barcelona.
This issue require to write some docs into the comments of the code.
Comment #8
sadamafridi commentedUpdated docblock in defaultStorageSettings() method to specify the need for unique setting names.
Updated docblock in defaultFieldSettings() method with similar instructions.
Comment #9
santhosh@21 commentedI have reviewed the documentation for the both methods and those are different from the ones returned by the other method and now it totally makes distinct and can avoid conflicts.
Comment #10
santhosh@21 commentedComment #11
joachim commented- * A list of default settings, keyed by the setting name.
+ * A list of default settings, keyed by the setting name. Each setting name
+ * must be unique to avoid conflicts when these arrays are merged by other
+ * components in the core, such as BaseFieldDefinition.
This doesn't describe the problem.
They are obviously unique, since they are array keys!
We need to say unique with the OTHER method.
There's no point mentioning BaseFieldDefinition explicitly.
Comment #12
joachim commentedSomething like this in the main body of the method docs:
> Setting names defined by this method must not duplicate the setting names returned by this plugin's implementation of OTHER METHOD, as both lists of settings are merged.
Comment #14
brandonlira commentedI've updated the documentation for the defaultStorageSettings() and defaultFieldSettings() methods to specify that setting names must be unique between them, as requested.
Thanks!
Comment #15
joachim commentedThe wording is good, but @note is not a docs tag supported in the list at https://www.drupal.org/docs/develop/standards/php/api-documentation-and-...
Comment #16
brandonlira commentedThanks for the feedback! I’ve removed the unsupported @note tag, kept the explanation in the standard comment format and fixed the coding standards.
Comment #17
joachim commentedThanks!
But all the @return docs must be a single paragraph -- see https://www.drupal.org/docs/develop/standards/php/api-documentation-and-....
Comment #18
brandonlira commentedI’ve adjusted the docblock formatting to ensure compliance with Drupal coding standards.
Thank you for your help!
Comment #19
smustgrave commentedGoing to go on a limb and believe this one is complete.
Comment #20
quietone commented@isalmanhaider, Instead of uploading screenshots of your IDE to bring attention to code, use a link to the file on GitLab, https://git.drupalcode.org/project/drupal.
Setting to needs work for #12. In that comment @joachim asked for the addition in the main part of the doc block. Right now, it is in the description of the return array.
Comment #22
brandonlira commentedComment #23
joachim commentedLooks great. Thanks!
Comment #24
catchCommitted/pushed to 11.x and cherry-picked to 11.2.x, thanks!