Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Status: Postponed » Active
xjm’s picture

Issue tags: +Block plugins
juanolalla’s picture

Status: Active » Needs review
FileSize
1.45 KB

Attaching patch with the params inline documentation.

juanolalla’s picture

I also added @params documentation for BlockBase::form(). Replace patch #3 with this.

Status: Needs review » Needs work

The last submitted patch, inline_documentation-1879396-4.patch, failed testing.

jhodgdon’s picture

The 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.

babruix’s picture

Status: Needs work » Needs review
FileSize
2.78 KB

Added inline documentation, please review.

babruix’s picture

Issue tags: +SprintWeekend2013
kaido.toomingas’s picture

Status: Reviewed & tested by the community » Needs review

Patch is tested by the community and seems to do what its designed to.

kaido.toomingas’s picture

Status: Needs review » Reviewed & tested by the community

add tag reviewd and tested

webchick’s picture

Status: Needs review » Needs work

Awesome, 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:

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -417,17 +417,20 @@ public function blockForm($form, &$form_state) {
+    // If machine_name was disabled.
     if (!empty($form['machine_name']['#disabled'])) {

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 just if () is a bit strange.)

You should check with her to be sure, but I'm pretty sure what xjm meant was more like:

   public function validate($form, &$form_state) {
+    // If machine_name was disabled, set its value to the name of the configuration object,
+    // e.g. foo of a file named blarg.foo.yml.
     if (!empty($form['machine_name']['#disabled'])) {
       $config_id = explode('.', $form_state['values']['machine_name']);
       $form_state['values']['machine_name'] = array_pop($config_id);
     }

(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().

babruix’s picture

Status: Needs work » Needs review
FileSize
1.98 KB

Corrected.

Status: Needs review » Needs work

The last submitted patch, drupal8.views-document-views-pager-base-plugin-1912650-12.patch, failed testing.

babruix’s picture

babruix’s picture

Status: Needs work » Needs review
xjm’s picture

Status: Needs review » Needs work

Thanks @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:

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -457,16 +460,18 @@ public function blockValidate($form, &$form_state) {}
   public function submit($form, &$form_state) {
+    // Only process if no errors found.
     if (!form_get_errors()) {
+      // Add block type-specific submission handling for the block form.
       $this->blockSubmit($form, $form_state);
 
       drupal_set_message(t('The block configuration has been saved.'));
+      // Mark cache items from all bins with content tag as invalid.
       cache_invalidate_tags(array('content' => TRUE));
+      // Set redirect back.
       $form_state['redirect'] = 'admin/structure/block/list/block_plugin_ui:' . $form_state['entity']->get('theme');

I'd suggest something like:

   public function submit($form, &$form_state) {
     // Process the block's submission handling if there were no errors.
     if (!form_get_errors()) {
       $this->blockSubmit($form, $form_state); 
       drupal_set_message(t('The block configuration has been saved.'));

       // Invalidate the content cache and redirect to the block listing.
       cache_invalidate_tags(array('content' => TRUE));
       $form_state['redirect'] = 'admin/structure/block/list/block_plugin_ui:' . $form_state['entity']->get('theme');

Try to do something similar with the first hunk in the patch as well.

xjm’s picture

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -417,17 +417,20 @@ public function blockForm($form, &$form_state) {
+    // If machine_name was disabled,
+    // set its value to the last part of machine_name
+    // e.g. foo of a machine_name test.fest.foo.
     if (!empty($form['machine_name']['#disabled'])) {

Also, 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).

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -417,17 +417,20 @@ public function blockForm($form, &$form_state) {
+    // All roles equal to false will be removed.

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.

babruix’s picture

Okay, how about this one

babruix’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal8.block-1879396-added-inline-documenation-18.patch, failed testing.

xjm’s picture

This is looking much better. A couple more suggestions:

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -417,19 +417,25 @@ public function blockForm($form, &$form_state) {
+    // If a machine name was not already set,
+    // change value to the last part of machine_name
+    // e.g. foo of a machine_name test.fest.foo.
     if (!empty($form['machine_name']['#disabled'])) {
       $config_id = explode('.', $form_state['values']['machine_name']);
       $form_state['values']['machine_name'] = array_pop($config_id);
     }

I'd suggest the comment simply say:

Remove the theme name prefix from the machine name for processing.

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 object block.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.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -417,19 +417,25 @@ public function blockForm($form, &$form_state) {
+    // Remove empty lines from the list of roles.
     $form_state['values']['visibility']['role']['roles'] = array_filter($form_state['values']['visibility']['role']['roles']);

This is much better! To clarify it further, how about:

Remove empty lines from the role visibility list.

Otherwise, I find myself asking "what list of roles?"

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -417,19 +417,25 @@ public function blockForm($form, &$form_state) {
+      // If entity is new, change form id to entity_theme.machine_name.
       form_set_value($form['id'], $form_state['entity']->get('theme') . '.' . $form_state['values']['machine_name'], $form_state);

This comment is still not quite correct. I'd suggest:

Generate the block instance ID from the theme and the machine name.

Note the capitalization of "ID" and the use of the word "the". :)

I think this is almost ready. Thanks!

babruix’s picture

Status: Needs work » Needs review
FileSize
2.67 KB

Ok, good spots, hopefully now it is ok.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.

Eclipse

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

I'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!

babruix’s picture

Status: Needs work » Needs review
FileSize
2.76 KB

Oh 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.

jhodgdon’s picture

Status: Needs review » Needs work

Sorry, 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)

+    // If editing of machine name was disabled.
     if (!empty($form['machine_name']['#disabled'])) {

...
+
+    // For new blocks only.
     if ($form_state['entity']->isNew()) {

These comments are not necessary. I can tell what the code is doing immediately from the lines of code.

b)

     if (!empty($form['machine_name']['#disabled'])) {
+
+      // Get machine name  from original value (without prepended theme name)
       $config_id = explode('.', $form_state['values']['machine_name']);
       $form_state['values']['machine_name'] = array_pop($config_id);

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.

babruix’s picture

Status: Needs work » Needs review
FileSize
3.08 KB

I`ve changed patch following your suggestions. If you still do not understand something, please, tell exact line or what is still unclear.

jhodgdon’s picture

Priority: Minor » Normal
Status: Needs review » Needs work

Thanks! This is looking better!

a) I like the part you added to the validate() doc block -- very helpful!

+   * Before send block to validation function BlockBase::blockValdiate(),
+   * need some preparation of $form_state array:
+   * - add machine_name to values array if if was not sent,
+   * - clear empty values from roles list,
+   * - set ID for new blocks.

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:

+      // Invalidate the content cache and redirect to the block listing,
+      // because need to remove cached block contents for each cache backend.

2nd line --> because we need...

Other than that, looks great, thanks!

jhodgdon’s picture

Oh 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!

babruix’s picture

Status: Needs work » Needs review
FileSize
3.22 KB
xjm’s picture

Thanks @babruix, @jhodgdon.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -414,22 +414,30 @@ public function blockForm($form, &$form_state) {
+   * - Add machine_name to values array if if was not sent.

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.

jhodgdon’s picture

Status: Needs review » Needs work
babruix’s picture

Status: Needs work » Needs review
FileSize
3.16 KB

Ok, removed this line:
Add machine_name to values array if if was not sent.

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +Blocks-Layouts, +Block plugins, +SprintWeekend2013

The last submitted patch, drupal8.block-1879396-added-inline-documenation-33.patch, failed testing.

jibran’s picture

Issue tags: +Needs reroll

Tagging.

babruix’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.1 KB

Status: Needs review » Needs work

The last submitted patch, drupal8.block-1879396-added-inline-documenation-37.patch, failed testing.

jibran’s picture

Title: Add inline documentation to BlockBase::validate() and BlockBase::submit() » Add inline documentation to BlockBase::validateConfigurationForm() and BlockBase::submitConfigurationForm()

Thank you @babruix for the reroll.
BlockBase::validate() and BlockBase::submit() are renamed to BlockBase::validateConfigurationForm() and BlockBase::submitConfigurationForm(). Please revert the changes for function names and updated the documentation accordingly.

babruix’s picture

Status: Needs work » Needs review
FileSize
1.35 KB

Seems most of unclear code, that required inline documentation, was removed, so in this patch only comments on submitConfigurationForm left.

jhodgdon’s picture

Where was that code moved to? Probably the documentation that is needed needs to be put there?

babruix’s picture

Code was removed in revision f630b51: Issue #1927608 by EclipseGc, tim.plunkett, effulgentsia: Remove the tight coupling between Block Plugins and Block Entities.

jibran’s picture

@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) and BlockFormController class holds blocks entity (instance related configuration and validations) related data. But I don't think BlockFormController::validate and BlockFormController::submit needs docs as these methods are inherited by EntityFormController.

jhodgdon’s picture

This 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?

babruix’s picture

these methods are inherited by EntityFormController

And base functions EntityFormController::validate() and EntityFormController::submit() are documented.
So no reasons for adding inline documentation for BlockBase::validate() and BlockBase::submit()

jhodgdon’s picture

Status: Needs review » Needs work

Here is what the issue summary for this issue says (which came from January 2013):

This is a followup issue for #1535868: Convert all blocks into plugins

BlockBase::validate() and BlockBase::submit() are lacking inline comments explaining the code flow.

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?

babruix’s picture

Status: Needs work » Needs review
FileSize
3.01 KB

Thanks for clarifying, Jennifer.
Added documentation in new places and removed @todo comments.

babruix’s picture

jhodgdon’s picture

OK, now we're talking. Can someone please review the accuracy of these inline comments? Thanks!

jhodgdon’s picture

Status: Needs review » Fixed

I decided it was OK and it had sat long enough. Committed to 8.x.

Automatically closed -- issue fixed for 2 weeks with no activity.