Problem/Motivation
It looks like there's quite a bit of refactoring of logic/code still planned for the Settings Tray module.
Before BigPipe was marked stable, we marked all classes & interfaces @internal
, because BigPipe provides only functionality, not APIs.
I think that you probably want to do something similar for Settings Tray? I think you want to limit the API surface in a very clear, very simple way:
Any block that provides an
off_canvas
form in their block plugin annotation, is automatically picked up.
(Plus probably something for making things other than blocks configurable via the Settings Tray.)
Proposed resolution
- Mark as much as possible as
@internal
. That would include all PHP code, all HTML (Twig templates), all JS, all CSS. The only thing that's an API, is the declaration of "off_canvas
forms". - Add a
outside_in.api.php
file to document the actual API.
(And yes, off_canvas
should be renamed to settings_tray
, but that's probably part of #2803375: Rename Outside-in module to "Settings Tray" for real.)
Remaining tasks
TBD
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#21 | 2894584-21.patch | 3.44 KB | tedbow |
#21 | interdiff-13-21.txt | 5.22 KB | tedbow |
#14 | 2894584-13.patch | 8.56 KB | Wim Leers |
#7 | interdiff-2894584-4-7.txt | 721 bytes | GrandmaGlassesRopeMan |
#7 | 2894584-7.patch | 10.07 KB | GrandmaGlassesRopeMan |
Comments
Comment #2
Wim LeersThis will prevent much of the future pain I mentioned in http://wimleers.com/talk/bc-burden-benefit and http://wimleers.com/article/simplicity-maintainability-cdn-module-drupal-8
Comment #3
Wim LeersThis would mean that big tasks like #2784935: Use Backbone for client-side state management can happen even after the module is marked stable.
Comment #4
tedbowHere is a patch.
Comment #5
tedbowand
The way we are marking form as off_cavnvas is in outside_in_block_alter but this is because these are existing blocks.
For a module that define it's own blocks it would be more correct to update add an 'off_canvas' form handler in their blocks plugin file.
How would that be documented in an .api.php file?
Comment #6
GrandmaGlassesRopeManJSDoc does not use
@internal
. I'd suggest using@private
or@protected
depending on the actual goal of this documentation.Comment #7
GrandmaGlassesRopeMan- Switch JSDoc to use
@private
, http://usejsdoc.org/tags-private.htmlComment #8
GrandmaGlassesRopeManComment #9
xjmNot RTBC-blocking, but we should definitely have a framework manager review this one.
Are there supposed to be double blank lines here?
Comment #10
xjmIf it's purely internal code, then why is there an interface for it?
Comment #11
xjm@xjm is apparently a broken record. See #2835604-14: BigPipe provides functionality, not an API: mark all classes & interfaces @internal and @Wim Leers' reply in #15 following.
Comment #12
xjm#2835758: Remove BigPipeInterface and move all of its docs to the implementation is the issue where BigPipe was actually de-interfaced.
Comment #13
Wim Leers#5
See
core/core.api.php
for plenty of examples of how to describe things in words, and not just usinghook_modulename_blah()
.#10++ — could not agree more.
Comment #14
Wim Leers#2844261: Allow dialog links to specify a renderer in addition to a dialog type landed, which renamed
OffCanvasRender
toOffCanvasRenderer
.#2784853: Determine when Outside In library should be loaded: piggyback on contextual_toolbar() landed, which removed
OutsideInManager(Interface)
andOutsideInCacheContext
.Straight reroll.
Comment #15
xjmCan we remove that interface, or spin off a separate issue to do so? Marking an interface
@internal
just seems like conflicting information, or "letter but not the spirit", outside of some weird/edgecase scenarios. Ideally we'd do both this and that before marking Settings Tray beta and I guess it wouldn't need to block this (we did it as a followup for BigPipe) but it does cause mental discordance for me in this patch. :PComment #16
Wim Leers#15: #2844261: Allow dialog links to specify a renderer in addition to a dialog type removed that interface altogether, so it's already taken care of :)
Comment #17
Wim LeersAFAICT this is ready. I have nits:
I think these deletions are accidental.
@xjm already asked about this in #9.
… but they're not commit-blocking IMHO. Especially not for
@internal
things.Comment #18
xjmLet's
get that followup filed andremove theblank lineswhitespace changes at least. :)Edit: I fail at reading. @Wim Leers clarified that the interface has already been removed and I confirmed this in the codebase. :)
Edit 2: But let's still remove the whitespace changes. It's fine to leave it at RTBC for such a trivial cleanup, but leaving things for committers to do on commit can be error-prone and adds more work to the commit process. :)
Comment #19
xjmAlso a note that I'm not totally confident about the way we're marking the templates internal. That pattern doesn't exist anywhere in core. Bartik and Seven are considered internal, but that's indicated in their
.info
files rather than their templates. Similarly, I didn't find any examples of CSS files being marked internal in this way. I pinged @lauriii and @Cottser about it.Comment #20
xjmDiscussed this with @Cottser. I also realized that module templates and CSS are considered internal anyway, by default, and we don't have a best practice for marking that yet anywhere. So let's remove the CSS and template hunks from this patch, and just keep the PHP and JS hunks.
I posted about both that question and the JS
@private
use here: #2550249: [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentationComment #21
tedbow@xjm ok here is just the PHP and JS files.
re #17 and #18.2 all extra lines where in CSS files so not problem anymore
Comment #22
Wim Leers#20: Thanks for explicitly confirming that templates and CSS are always internal. I'd rather see the patch try to mark too much internal and then a committer marking NW for marking too much internal than marking too little, because then the record (this issue) documents the intent in case rules change later :)
Back to RTBC, #21 looks great!
Comment #24
xjmForgot to save retitling the issue before commit.
Comment #26
xjmCommitted to 8.4.x, thanks!
An 8.3.x backport would look different because of the JS file locations. It could be backported since it's just documenting the correct state of this functionality, but leaving against 8.4.x now.
Comment #28
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)