- ๐บ๐ธUnited States nicxvan
I think this is ready, the extensive feedback has been addressed!
- ๐บ๐ธUnited States fathershawn New York
@nicxvan First, thank you for so carefully checking the tedious mundane details! Suggestions committed and test updated for the one method name fix.
Sanitization happens in the attribute value object's
::__toString
method. They all extend AttributeValueBase which sanitizes the name property. We addAttributeJson
in this MR, and follow this pattern. - ๐บ๐ธUnited States nicxvan
Ok that was tedious, but I clicked on all documentation links and confirmed they align except the ones that I have suggestions for.
I also reviewed all changes and have been following. I think this is ready assuming the three suggestions are accepted and tests pass.
Only remaining substantial question I have is where does sanitization happen?
I see some attributes do check the expected value is a boolean, but I don't see anything else, is that all handled upstream of this? - ๐ฌ๐งUnited Kingdom catch
Given @larowlan's:
I did a search of contrib for classes that extend this where these new return types might be a BC break and could only find on in display suite, that was only adding a new method - and one in htmx which also didn't override any of the methods we're adding too here So I think this change is fine to do in a minor release, with a change record
and https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... โ I think we're OK to add the return type hints to
Attribute
here with a change record. It's allowed under the bc policy, and we can be fairly confident it won't break any existing code. - ๐บ๐ธUnited States fathershawn New York
Thank you @phenaproxima for improving the code with your questions and suggestions. I've resolved all comments with either updates or answers except those related to return types (see #30). All tests are passing.
The Needs Review Queue Bot โ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide โ to find step-by-step guides for working with issues.
- ๐บ๐ธUnited States fathershawn New York
@catch can you give guidance about adding return types to
\Drupal\Core\Template\Attribute
?I hit a name collision on the BC policy with the Attribute name (As you and @nicxvan noted that was for PHP attributes). But it also seems to be permitted with these:
PHP and JavaScript classes โ
In general, only interfaces can be relied on as the public API. With the exception of base classes, developers should generally assume that implementations of classes will change, and that they should not inherit from classes in the system, or be prepared to update code for changes if they do. See also more specific notes below.Public methods not specified in an interface โ
Public methods on a class that aren't on a corresponding interface are considered @internal.We are adding an interface in this issue, but there no interface for this class now. @larowan seemed to think it was okay here: https://git.drupalcode.org/project/drupal/-/merge_requests/12097#note_51...
- ๐บ๐ธUnited States smustgrave
With regards to documentation updates how do we queue that up?