Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
This is a followup issue for #1535868: Convert all blocks into plugins and postponed on that issue.
BlockBase::validate()
andBlockBase::submit()
are lacking inline comments explaining the code flow.
Proposed resolution
- Add inline documentation to
BlockBase::validate()
andBlockBase::submit()
to make the code more readable.
Comments
Comment #1
xjmComment #2
xjmComment #3
juanolalla CreditAttribution: juanolalla commentedAttaching patch with the params inline documentation.
Comment #4
juanolalla CreditAttribution: juanolalla commentedI also added @params documentation for BlockBase::form(). Replace patch #3 with this.
Comment #6
jhodgdonThe test failure is not related to this patch.
So, we don't actually normally document $form and $form_state parameters on form constructors. Are these methods considered to be form constructors? If so, and if their documentation follows the form constructor standards, we don't need to add these @param lines. See
http://drupal.org/node/1354#forms
Anyway... what this issue is actually asking for is *inline* comments, meaning comments inside the code explaining why it is doing what it's doing.
Comment #7
babruix CreditAttribution: babruix commentedAdded inline documentation, please review.
Comment #8
babruix CreditAttribution: babruix commentedComment #9
kaido.toomingas CreditAttribution: kaido.toomingas commentedPatch is tested by the community and seems to do what its designed to.
Comment #10
kaido.toomingas CreditAttribution: kaido.toomingas commentedadd tag reviewd and tested
Comment #11
webchickAwesome, thanks for working on this!
So I didn't make this issue so I'm not precisely sure what xjm had in mind here, but methinks it was something more along the lines of explaining WHY the code works the way it does, rather than HOW the code works. For example:
I'm the biggest documentation fan you'll ever find, but that's too much even for me. :) I can tell that simply by reading the line below. (Although why it's
if (!empty())
instead of justif ()
is a bit strange.)You should check with her to be sure, but I'm pretty sure what xjm meant was more like:
(Note: I have no idea if that's accurate or not since I'm not in the code, but that general thing is what we want.)
Then, as a totally pedantic thing, documentation lines should end in a period. Most of them do, but there are a couple of stragglers in submit().
Comment #12
babruix CreditAttribution: babruix commentedCorrected.
Comment #14
babruix CreditAttribution: babruix commentedUps
Comment #15
babruix CreditAttribution: babruix commentedComment #16
xjmThanks @babruix!
I'd agree with what @webchick says in #11. We don't want to duplicate the information that's on every line of code; we want to explain what it's doing and why at a higher level, so that someone can scan and understand what the code is doing. Adding comments for individual lines generally makes the code harder to read, not easier.
Also, in general, inline documentation should be complete sentences with capitalization, periods, and all the little words you'd include normally. (Note that "ID" should always be capitalized in comments--id is something else.)
So, for example, instead of adding all these individual comments:
I'd suggest something like:
Try to do something similar with the first hunk in the patch as well.
Comment #17
xjmAlso, this comment is incorrect. It's not happening "If machine_name was disabled"; it's happening when the machine name field is not disabled, that is, if a machine name was not already set (since you can't change the machine name of a block instance that has already been placed).
This comment is also confusing. Roles equal to false? I'd suggest describing what the code is actually doing, instead--removing empty lines from the list of roles.
Comment #18
babruix CreditAttribution: babruix commentedOkay, how about this one
Comment #19
babruix CreditAttribution: babruix commentedComment #21
xjmThis is looking much better. A couple more suggestions:
I'd suggest the comment simply say:
Note that this hunk has some code smell, which is why it's difficult to come up with a good code comment. :) Right now what happens is that you set the machine name to
my_machine_name
and then the block system creates a configuration objectblock.block.my_theme.my_machine_name
for the block instance. Currently, the theme name is exposed to the user when editing the block instance, which is confusing since the user never entered it. There may be an existing issue to fix this--if not, let's file a followup.This is much better! To clarify it further, how about:
Otherwise, I find myself asking "what list of roles?"
This comment is still not quite correct. I'd suggest:
Note the capitalization of "ID" and the use of the word "the". :)
I think this is almost ready. Thanks!
Comment #22
babruix CreditAttribution: babruix commentedOk, good spots, hopefully now it is ok.
Comment #23
EclipseGc CreditAttribution: EclipseGc commentedThis looks good to me.
Eclipse
Comment #24
jhodgdonI'm really not comfortable with the comments here. As mentioned by webchick in #11, it's generally not necessary or even a good idea for comments to tell you what the code is doing. What we want is to know why the code is doing things.
For instance, the first comment in the validate method might explain that (I think?) the value reaching this function is composed of theme name . machine name, and we want to break it down to just machine name so that we can prepend a different theme name later in the function, and it would also be useful to explain why we would be doing this in the first place, which is not ever explained in the code comments at all.
So I'm sending this back for more work... sorry!
Comment #25
babruix CreditAttribution: babruix commentedOh how it`s hard to satisfy you :)
I`ve played with debugger (to understand better what is going on there) and providing patch with investigation results.
Comment #26
jhodgdonSorry, I'm not trying to be difficult! We are just looking for comments that will actually help people maintain and read/understand the code. The comments in the submit() function now look pretty good to me, although it might be helpful to have a "because..." after "invalidate the content cache" to explain why this is necessary. I don't really know why that would be, off-hand.
The validate() function comments are also still not ready, in my opinion:
a)
These comments are not necessary. I can tell what the code is doing immediately from the lines of code.
b)
This comment is good, because it tells me something that isn't obvious. However, I don't think we need a blank line before the comment, and it has an extra space in the middle, and it needs to end with ".".
c) In general, after looking at the patched validate() code, I am still not sure why all of this stuff is taking place -- which would be a good thing to comment on (again, see #11 above). A comment (perhaps in the documentation block above?) about what the purpose of the whole function is would be helpful -- because to me it doesn't appear it is just validating, as the name would suggest. Instead it appears to be redefining the machine name (why?), setting up an ID (and I have no idea what the ID is actually used for from these comments) and cleaning up the roles array.
Comment #27
babruix CreditAttribution: babruix commentedI`ve changed patch following your suggestions. If you still do not understand something, please, tell exact line or what is still unclear.
Comment #28
jhodgdonThanks! This is looking better!
a) I like the part you added to the validate() doc block -- very helpful!
But it needs some grammatical and style attention:
- Before send block -> Before sending the block
- blockValidate() is misspelled
- need some preparation of -> we need to prepare
- List items should start with a capital letter and end with a "." see http://drupal.org/node/1354#lists
b) The very last comment in the patch also needs a bit of grammatical attention:
2nd line --> because we need...
Other than that, looks great, thanks!
Comment #29
jhodgdonOh yeah, in (a) I think this should be a separate paragraph from the documentation that is already there, so leave a blank line after the new parts. Thanks!
Comment #30
babruix CreditAttribution: babruix commentedComment #31
xjmThanks @babruix, @jhodgdon.
This comment is incorrect. As I've stated above, what it's actually checking to see if the machine name field is disabled (that is, already set), and overriding its value to remove the theme prefix if so. The reason for this is that when the block instance is new, the user enters a machine name, and the theme name is automatically appended to it to create the configuration object object name, but when an existing block instance is rendered, it includes the theme prefix despite that the user never entered it. This is a fragile behavior and I believe there's an existing issue for it, but for now, let's at least not add misleading documentation.
Comment #32
jhodgdonComment #33
babruix CreditAttribution: babruix commentedOk, removed this line:
Add machine_name to values array if if was not sent.
Comment #34
jibran#33: drupal8.block-1879396-added-inline-documenation-33.patch queued for re-testing.
Comment #36
jibranTagging.
Comment #37
babruix CreditAttribution: babruix commentedComment #39
jibranThank you @babruix for the reroll.
BlockBase::validate()
andBlockBase::submit()
are renamed toBlockBase::validateConfigurationForm()
andBlockBase::submitConfigurationForm()
. Please revert the changes for function names and updated the documentation accordingly.Comment #40
babruix CreditAttribution: babruix commentedSeems most of unclear code, that required inline documentation, was removed, so in this patch only comments on submitConfigurationForm left.
Comment #41
jhodgdonWhere was that code moved to? Probably the documentation that is needed needs to be put there?
Comment #42
babruix CreditAttribution: babruix commentedCode was removed in revision f630b51: Issue #1927608 by EclipseGc, tim.plunkett, effulgentsia: Remove the tight coupling between Block Plugins and Block Entities.
Comment #43
jibran@jhodgdon in #1927608: Remove the tight coupling between Block Plugins and Block Entities we have an architectural change in blocks. Now
BlockBase
class holds blocks plugin related data(default settings and configuration of block) andBlockFormController
class holds blocks entity (instance related configuration and validations) related data. But I don't thinkBlockFormController::validate
andBlockFormController::submit
needs docs as these methods are inherited byEntityFormController
.Comment #44
jhodgdonThis issue was originally filed to add **inline** (in-code, not header) documentation to validate and submit methods.
If there is validation and submission handling happening somewhere, then that code is what needs to still be documented for this issue, right?
Comment #45
babruix CreditAttribution: babruix commentedAnd base functions
EntityFormController::validate() and EntityFormController::submit()
are documented.So no reasons for adding inline documentation for
BlockBase::validate() and BlockBase::submit()
Comment #46
jhodgdonHere is what the issue summary for this issue says (which came from January 2013):
So that other issue
#1927608: Remove the tight coupling between Block Plugins and Block Entities
took the code that was in BlockBase::validate() as of May, and moved it to BlockFormController::validate(). And by that point, there wasn't much in BlockBase::submit(), so who knows why that part of this issue was there...
But In any case, if the code in BlockBase::validate() needed inline documentation when this issue was filed, it probably still needs inline documentation now in BlockFormController::validate(), and this patch does not take care of it. Reading the doc... well I'm not sure exactly why it is doing what it is doing, so I think some docs might not hurt, especially near the top. The BlockFormController::submit() method seems to have pretty good inline docs now.
So could we fix up the docs for the code that was actually pointed to in this original issue, as well as removing the @todo comments that this patch is doing?
Comment #47
babruix CreditAttribution: babruix commentedThanks for clarifying, Jennifer.
Added documentation in new places and removed @todo comments.
Comment #48
babruix CreditAttribution: babruix commentedFixed misspelling.
Comment #49
jhodgdonOK, now we're talking. Can someone please review the accuracy of these inline comments? Thanks!
Comment #50
jhodgdonI decided it was OK and it had sat long enough. Committed to 8.x.