Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xenophyle’s picture

Status: Active » Needs review
FileSize
11.66 KB

Here's my first pass at a patch.

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good, thanks!

A few things to fix:

a)

 /**
- * Form submission handler for the main blocks administration form.
+ * Form submission handler for the main block administration form.

See http://drupal.org/node/1354#forms for standards on how to document form submission headers. Also, farther down in the patch, this form generating function should use the standards:

 /**
- * Define the custom block form.
+ * Defines the custom block form.

b)

+ *
+ * @param @a, @b
+ *   Both parameters are blocks represented by arrays

- Each parameter gets a separate @param
- Description ends in .
- We also have standards for how to document sorting callbacks like this -- see http://drupal.org/node/1354#functions (scroll down to "Callbacks" section). In particular, you probably don't need the @param/@return at all on this function; instead, include the "Callback for uasort in function_name_here()." line.

c) Right above that:

  *
  * Active blocks are sorted by region, then by weight.
  * Disabled blocks are sorted by name.

This could be wrapped better - should go out to as close to 80 characters as possible without going over.

d)

 /**
  * Menu item access callback - only admin or enabled themes can be accessed.
+ *
+ * Path: 

This should say "Access callback: only...". See http://drupal.org/node/1354#menu-callback -- also the next section in the patch should be "Theme callback: ...".

e)

  * @param $theme
  *   The theme whose blocks are being configured. If not set, the default theme
  *   is assumed.
+ * 
  * @return

There is an extra space at the end of the added line. You can probably set your editor to delete trailing whitespace, or at least to show trailing whitespace. I didn't necessarily look at the whole patch carefully for extra whitespace yet, so there may be other places.

f)

/**
- * Implements hook_form_FORM_ID_alter().
+ * Implements hook_form_FORM_ID_alter() for user_profile_form.

Whenever you use a function name, put () after it (this signals the API module to turn it into a link on api.drupal.org).

g)

 /**
- * Build the content and subject for a block. For cacheable blocks, this is
+ * Builds the content and subject for a block. For cacheable blocks, this is
  * called during #pre_render.

This needs to be split into two paragraphs. The first line of any docblock should be one 80-character-or-less sentence.

xenophyle’s picture

Wow, thanks for the quick response! I'll get to work.

xenophyle’s picture

Status: Needs work » Needs review
FileSize
14.82 KB

Trying again...

jhodgdon’s picture

Status: Needs review » Needs work

This is looking much better! A few minor things to fix:

a)

+ * Path: admin/structure/block/demo% (for each theme)

Path needs / in it: admin/structure/block/demo/%

b)

+ * Path: admin/structure/block.

Having a . here seems a bit confusing to me and isn't in our standards for menu callbacks.

c)

+ * Callback for uasort in function_name_here().

- uasort should be uasort() to turn into a function link on api.drupal.org
- Substitute the actual function where this is used for function_name_here(). I guess my comment above was taken a bit too literally. Check out the standards at http://drupal.org/node/1354#functions (scroll down to the section called "Callback functions" within that section).

d)

+ * Theme callback: Uses the theme specified in the paramter.

spelling: paramter -> parameter

xenophyle’s picture

+ * Callback for uasort in function_name_here().

Ok, I truly do not know how that ended up in here; I have a definite memory of editing that line to actually make sense. :)

xenophyle’s picture

Assigned: Unassigned » xenophyle
Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

+ * Callback for usort in block_admin_display_prepare_blocks().

usort() still needs parens to turn into a function link. Fix that and I think it's good to go!

xenophyle’s picture

Here's the patch with that change.

Thanks for all your help!

xenophyle’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

xenophyle’s picture

I just noticed that there is an @ingroup hooks line in the sample hook definition. Is that something that should go in all modules?

jhodgdon’s picture

Yes, all hooks are supposed to have that, so that they appear on the Hooks topic page on api.drupal.org. Or else they should be within an @ingroup hooks @{ @} section. It wasn't specifically part of the cleanup... do you see some hooks in this module that are missing from the Hooks topic list?

xenophyle’s picture

Ah, I didn't realize the @ingroup hooks @{ @} section did the same thing. I guess that's all set, then.

xenophyle’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
15.78 KB

I just changed the way form constructors that are also used in page callbacks are documented as per #1325116: Clean up API docs for Aggregator module.

xjm’s picture

Status: Needs review » Needs work

Hey @xenophyle. Looks like this patch is in pretty good shape! Here are a few minor things I noticed:

+++ b/modules/block/block.moduleundefined
@@ -428,9 +446,9 @@ function _block_rehash($theme = NULL) {
-    // Preserve info which is not in the database.
+    // Preserve info that is not in the database.
...
-    // The cache mode can only by set from hook_block_info(), so that has
+    // The cache mode can only be set from hook_block_info(), so that has

Since these comments aren't in the doxygen blocks, it might be better to correct them in a separate issue. Maybe it's okay, but adding other cleanups makes it more likely that this patch will collide with other patches. Reference: http://webchick.net/please-stop-eating-baby-kittens

+++ b/modules/block/block.moduleundefined
@@ -511,7 +530,13 @@ function block_custom_block_get($bid) {
+ *   Associative array of information retrieved by block_custom_block_get() for
+ *   this block if editing an existing block, otherwise an empty array.

Two suggestions here: 1. While we are adding this parameter documentation, do we want to make it use the normal pattern for optional parameters? 2. It seems like a bit of a run-on sentence.

How about this?

(optional) An associative array of information retrieved by block_custom_get_block() if an existing block is being edited, or an empty array otherwise. Defaults to array().

Edit: Or, we could split it into two sentences. What do you think?

+++ b/modules/block/block.moduleundefined
@@ -744,8 +776,8 @@ function _block_load_blocks() {
+ * Checks the page, user role and user specific visibilty settings.
+ * Removes the block if the visibility conditions are not met.

I think this should have a comma after "user role" for clarity, and a hyphen in "user-specific." Minor nitpick, though. :)

+++ b/modules/block/block.moduleundefined
@@ -900,9 +934,9 @@ function block_flush_caches() {
- * Process variables for block.tpl.php
+ * Processes variables for block.tpl.php

Missing a period at the end.

+++ b/modules/block/block.tpl.phpundefined
@@ -12,11 +12,12 @@
+ *   preprocess functions. The default values can be one or more of the ¶

Trailing whitespace left over from rewrapping here.

+++ b/modules/block/block.tpl.phpundefined
@@ -12,11 +12,12 @@
+ *   - block-[module]: The module generating the block. For example, the user
+ *     module is responsible for handling the default user navigation block. In
+ *     that case the class would be "block-user".

Totally a minor point/nitpick, but I'd use single quotes here, since that's more usual for denoting a literal value.

xjm’s picture

Note that this patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

xenophyle’s picture

Status: Needs work » Needs review
FileSize
15.23 KB

Rerolled patch following xjm's suggestions. Thanks for the 8.x restructuring alert.

xjm’s picture

I've read through #18 and I think this is RTBC if it passes testbot. Thanks @xenophyle!

xjm’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

xenophyle’s picture

Status: Closed (fixed) » Needs review
FileSize
954 bytes

Unclassify form constructors as page callback as per #1337282: Lacking doc header standard for forms used as page callbacks

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Yep. Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

xjm’s picture

Version: 8.x-dev » 7.x-dev
Assigned: xenophyle » Unassigned
Status: Closed (fixed) » Needs review
Issue tags: +Needs backport to D7
FileSize
9.75 KB
12.07 KB

With the menu callback stuff removed. This one had just one hunk that didn't apply, but I couldn't find the function in D7.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine for d7, thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

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