Problem/Motivation

Part of meta-issue #1310084: [meta] API documentation cleanup sprint

Proposed resolution

Correct the following in the core dblog module:

  • Ensure that all @return lines are preceded by a blank line.
  • Ensure that @see and @ingroup lines are always at the end of docblocks.
  • Ensure that all lines in doxygen blocks are 80 characters or fewer (excluding the terminal newline).
  • Ensure that all functions, classes, interfaces, and methods have one-line summaries that are clear, use the correct verb tense, and follow specific standards in http://drupal.org/node/1354.
  • Make incidental style and grammar corrections only to those docblocks already being updated.

Remaining tasks

Patch needs to be rolled.

User interface changes

None.

API changes

None.

Follow-up issues

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Status: Active » Needs review
FileSize
23.86 KB

Attached is a first pass at bringing the dblog module up to Drupal 8 documentation standards. I relaize this patch will need to be re-rolled once the files are moved to /core. However, comments in the meantime are welcome.

While creating this patch, I encountered a number of questions which also may necessitate re-rolling this patch. My questions include:

  1. What is the correct method of documenting '@see function_foo()'? Should there be a period at the end of that line or not?
  2. Should members of an associative array have their variable type documented in a specific manner?

    For instance, should the variable $variables in theme_dblog_message() be documented something akin to the following?

     * @param array $variables
     *   An associative array containing:
     *   - event: (object) An object with at least the message and variables properties.
     *   - link: (string) (optional) Format message as link, event->wid is required.
     *
    
  3. Should there be a blank line between @see and @ingroup directives?
  4. Is there a standard for whether a blank line should appear in code before the 'return $foo' at the end of a function? I ask because a blank line occurs right before the end of dblog_schema().
  5. There does not seem to be documentation standard indicating that a function is covered by either a specific UnitTestCase or WebTestCase. For instance, how should one document that the entries in dblog_menu
    are covered in tests in DBLogTestCase::verifyReports()? Should one add '@see DBLogTestCase::verifyReports()' at the end of the docblock?
  6. Do derived WebTestCase classes require a docblock?

Known issues requiring follow-up

  1. The path /admin/reports/search is defined in this module, but there is no test confirming that it is reachable. Such a test needs to be added to this module with simulated search events.
jhodgdon’s picture

In answer to your questions:

1) @see - no . -- this is on http://drupal.org/node/1354 (search for @see on the page)
2) We are not cleaning up data types in these cleanup patches. And we don't have a standard to answer the question you posed - can you please file an issue to discuss?
3) No blank line is necessary.
4) We're not fixing up coding standards, only docs standards.
5) I don't think functions should have @see to point to their tests.
6) That is under discussion. There are two different standards that conflict. See issue:
#338403: Use {@inheritdoc} on all class methods (including tests)
(latest comment that I added a few minutes ago)

Lars Toomre’s picture

@jhodgdon Thanks for the answers to my questions in item #1. I will go back and review this patch in the next couple of days after /core files are moved. Also to answer your question in a couple of other issues, I currently am working on patches for documentation API clean up in three of the modules (dblog, image and locale) as well as the lists formatting issue. I will turn to other modules once these four issues are further along.

jhodgdon’s picture

Status: Needs review » Needs work

OK. Going forward, it would probably be best to wait on filing the issues until you're ready to work on them. Also, one of them didn't get into the list on the meta issue.

I also took a quick look at part of your patch and noticed a couple of other things to address:

a) It looks like it needs a few "the", "of", etc. words in the one-line function descriptions. For instance:

Creates list dblog administration filters that can be applied.
==>
Creates a list of dblog administration filters that can be applied.

b) Two dimension array ==> Two-dimensional array

c) Please don't touch the code lines, such as:

   }
-
   if (!empty($types)) {

This cleanup should be documentation fixes only.

d)
Returns a HTML-formatted ... => an HTML
(this needs to be fixed in several places)

e) Most of the form generating function docs are cleaned up correctly. Missed this one:
Submission callback for dblog_clear_log_form().
(should be Submission handler for...)

f) " The dblog module monitors ..." This should either be "The Database logging module" or "dblog.module".

g) Regarding your @todo -- yes, each class, function, method, constant, etc. requires a docblock.

jhodgdon’s picture

Issue tags: +Novice, +Needs backport to D7

Lars Toomre: Are you still planning to work on this? If so, please respond. If not, we'll unassign it so someone else can.

Also, tagging.

Lars Toomre’s picture

I thought that I had unassigned myself from this already. I am not working on this and do not plan to.

xjm’s picture

Assigned: Lars Toomre » Unassigned
kid_icarus’s picture

Rerolled #1. This patch applies cleanly at commit 5e0eac1.

kid_icarus’s picture

Status: Needs work » Needs review
kid_icarus’s picture

This should address everything in #4 :)

jhodgdon’s picture

Status: Needs review » Needs work

Hi kit_icarus -- thanks for taking this on! I found a few things that need to be fixed in this patch:

a) In between the time of this first patch and now, the standards for hook_menu() callbacks have changed, and we're not putting in lines like:

+ * Path: admin/reports/dblog

any more. Current standards are at:
http://drupal.org/node/1354#menu-callback

b) Right at the top of the patch:

 /**
- * Menu callback; generic function to display a page of the most frequent events.
+ * Page callback: Displays the most frequent unique messages of a given event
+ * type.

The first sentence needs to be compressed into one 80-character-or-less line, see
http://drupal.org/node/1354#general

c)

@@ -240,13 +267,16 @@ function dblog_filters() {
 }
 /**
- * Returns HTML for a log message.
+ * Returns an HTML string for a dblog event.
  *
- * @param $variables
+ * @param array $variables
  *   An associative array containing:
  *   - event: An object with at least the message and variables properties.
  *   - link: (optional) Format message as link, event->wid is required.
  *
+ * @return string
+ *   An HTML string.
+ *
  * @ingroup themeable
  */
 function theme_dblog_message($variables) {

Should follow standards at http://drupal.org/node/1354#themeable (doesn't need @return).

d) Another standards change:

@@ -6,9 +6,9 @@ use Drupal\Core\Database\Database;
  * @file
  * System monitoring and logging for administrators.
  *
- * The dblog module monitors your site and keeps a list of
- * recorded events containing usage and performance data, errors,
- * warnings, and similar operational information.
+ * The Database logging module monitors your site and keeps a list of recorded

Should be "Database Logging module" (capitalization).

e)

@@ -98,7 +98,7 @@ function dblog_init() {
 /**
  * Implements hook_cron().
  *
- * Remove expired log messages and flood control events.
+ * Controls size of dblog table paring it to 'dblog_row_limit' message count.

This last sentence is very terse. Add "the", punctuation, etc. to make it read better.

f)

@@ -160,7 +166,7 @@ function dblog_watchdog(array $log_entry) {
 }
 
 /**
- * Implements hook_form_FORM_ID_alter().
+ * Implements hook_form_FORM_ID_alter() for system form.
  */
 function dblog_form_system_logging_settings_alter(&$form, $form_state) {

First line should follow 2nd example in this section http://drupal.org/node/1354#hookimpl

g)

+++ b/core/modules/dblog/dblog.test
@@ -5,6 +5,9 @@
  * Tests for dblog.module.
  */
 
+/**
+ * Tests the Dblog module functionality.
+ */
 class DBLogTestCase extends DrupalWebTestCase {

Should say "Database Logging module" here, in both places... Really, that should be done throughout the docs in this module instead of saying "dblog" when it refers to the module, and in other places, maybe it should say "database log" or just "log" if the non-word "dblog" is being used to refer to the log rather than the module.

h)

@@ -569,21 +601,24 @@ class DBLogTestCase extends DrupalWebTestCase {
   }
 
   /**
-   * Assert messages appear on the log overview screen.
+   * Confirms dblog message appears on the dblog overview screen.

Sentence is terse, needs "that" "the", etc.

kid_icarus’s picture

Status: Needs work » Needs review
FileSize
7.51 KB
23.35 KB

Thanks for the review!

Here are my attempts to address everything in #11. I found it difficult to limit one-line function descriptions to 80 meaningful characters without being terse, and it felt awkward writing the terms 'log' and 'database log' interchangably.

xjm’s picture

Status: Needs review » Needs work

Thanks @kid_icarus! This looks close to ready. I found only a couple minor things to fix:

  1. +++ b/core/modules/dblog/dblog.admin.incundefined
    @@ -367,12 +392,13 @@ function dblog_clear_log_form($form) {
    + * Submission handler for dblog_clear_log_form().
    + *
    + * @see dblog_clear_log_form()
    

    The @see is not needed here (since there is a link in the summary).

  2. +++ b/core/modules/dblog/dblog.moduleundefined
    @@ -98,10 +98,10 @@ function dblog_init() {
    +  // Cleanup the database log table.
    

    While we're changing this line (which is technically out of scope, along with all the other inline comment changes, but they are good changes and I guess it is OK to fix them here): "Clean up" should be two words when it is a verb.

  3. +++ b/core/modules/dblog/dblog.testundefined
    @@ -2,9 +2,12 @@
    + * Tests for Database Logging module.
    

    I think this should say "...the Database Logging module."

  4. +++ b/core/modules/dblog/dblog.testundefined
    @@ -407,15 +431,17 @@ class DBLogTestCase extends DrupalWebTestCase {
    -   * Test the dblog filter on admin/reports/dblog.
    +   * Tests the database log filter functionality.
    +   *
    +   * Path: admin/reports/dblog
    

    For this, I think we can say:
    Tests the database log filter functionality at admin/reports/dblog.

  5. +++ b/core/modules/dblog/dblog.testundefined
    @@ -569,21 +602,24 @@ class DBLogTestCase extends DrupalWebTestCase {
    +   *
    +   * Path: /admin/reports/dblog
    

    The path line should be removed here (we reverted that addition to the documentation standard).

Also, can whoever rerolls the patch with these changes include an interdiff along with the full patch? Thanks!

kid_icarus’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
23.27 KB

This patch addresses #13. I attached an interdiff as requested :)

Thanks for the review @xjm! Do you know of any docs that describe various udpates to the comment formatting conventions?

xjm’s picture

Status: Needs review » Needs work

Thanks @kid_icarus! The changes in the interdiff look good and address everything in my review. I did notice that there are a few stray points from @jhodgdon's review to address yet (not sure why I missed them before):

  1. +++ b/core/modules/dblog/dblog.admin.incundefined
    @@ -6,10 +6,12 @@
    - * Menu callback; displays a listing of log messages.
    + * Page callback: Displays a listing of dblog messages.
    
    @@ -349,10 +372,12 @@ function dblog_filter_form_submit($form, &$form_state) {
    + * This form allows the user to clear the dblog table of event messages.
    
    +++ b/core/modules/dblog/dblog.testundefined
    @@ -81,19 +90,20 @@ class DBLogTestCase extends DrupalWebTestCase {
    -    // Verify dblog row count equals row limit plus one because cron adds a record after it runs.
    
    @@ -375,15 +396,18 @@ class DBLogTestCase extends DrupalWebTestCase {
    +   * tests the clearing dblog functionality through the admin interface.
    
    @@ -472,17 +496,21 @@ class DBLogTestCase extends DrupalWebTestCase {
    +   *   List of dblog events where each event is an array with following keys:
    +   *   - severity: (int) A dblog severity constant.
    +   *   - type: (string) The type of dblog event.
    +   *   - message: (string) The message for this dblog event.
    +   *   - user: (string) The user associated with this dblog event.
    

    We should probably change these "dblog" to "log" or "database log" as we have done elsewhere.

  2. +++ b/core/modules/dblog/dblog.testundefined
    @@ -81,19 +90,20 @@ class DBLogTestCase extends DrupalWebTestCase {
    +    // Verify dblog row count equals row limit plus one because cron adds a
    +    // record after it runs.
         $count = db_query('SELECT COUNT(wid) FROM {watchdog}')->fetchField();
    
    @@ -179,14 +190,14 @@ class DBLogTestCase extends DrupalWebTestCase {
    +    // Add user using form to generate add user event (which is not triggered by
    +    // drupalCreateUser).
    
    @@ -213,7 +225,7 @@ class DBLogTestCase extends DrupalWebTestCase {
    -    // Delete user.
    +    // Delete user created at start of this test.
    
    @@ -221,9 +233,10 @@ class DBLogTestCase extends DrupalWebTestCase {
    -    // Default display includes name and email address; if too long then email is replaced by three periods.
    +    // Default display includes name and email address; if too long, the email
    +    // address is replaced by three periods.
    
    @@ -272,7 +286,8 @@ class DBLogTestCase extends DrupalWebTestCase {
    +    // Create node using form to generate add content event (which is not
    +    // triggered by drupalCreateNode).
    
    @@ -301,32 +316,35 @@ class DBLogTestCase extends DrupalWebTestCase {
    +    // Verify 'access denied' event was recorded.
    ...
    +    // Verify 'page not found' event was recorded.
    
    @@ -407,15 +431,15 @@ class DBLogTestCase extends DrupalWebTestCase {
    -    // Clear log to ensure that only generated entries are found.
    +    // Clear log to ensure that only generated entries will be found.
    
    @@ -460,8 +484,8 @@ class DBLogTestCase extends DrupalWebTestCase {
    +    // Set filter to match each of the two filter-type attributes and confirm
    +    // correct number of entries are displayed.
    

    These comments are still a bit terse and should have some articles ("a" or "the") added for clarity.

  3. +++ b/core/modules/dblog/dblog.testundefined
    @@ -120,9 +130,10 @@ class DBLogTestCase extends DrupalWebTestCase {
    -   * Verify the logged in user has the desired access to the various dblog nodes.
    +   * Verifies logged in user has desired access to various database log nodes.
    

    I looked up this method and I think this description wasn't actually accurate. I don't think they're actually nodes at all?

    Maybe:
    "Confirms that database log reports are displayed at the correct paths."

I also applied the patch locally and checked for any missed corrections. The CSS files need @file docblocks, and the docblock for dblog_clear_log_form() is not quite to standard for submission handlers. Reference: http://drupal.org/node/1354#forms

Finally, I noticed the patch does not apply currently, so it should be rebased before making further changes and the interdiff for them. Thanks!

xjm’s picture

Oh, regarding the history of our doc standards, there's two good resources I can think of: the revision history for 1354 and the general list of change notifications.

kid_icarus’s picture

Status: Needs work » Needs review
FileSize
23.25 KB

Re-rolled. Patch applies cleanly at b4c77c0.

kid_icarus’s picture

Once again, thanks for the review @xjm!

This should address #15, as well as other edits I saw as necessary.

filijonka’s picture

dblog.admin.inc
General: Functions/methods one-line summary should start with a verb in the right tense

function dblog_overview()
function dblog_top
function dblog_event
function theme_dblog_message
function dblog_filter_form
dblog_filter_form_submit
dblog_clear_log_form
* missing @return

function dblog_filter_form
function dblog_filter_form_validate
dblog_filter_form_submit
dblog_clear_log_form
* missing @params

dblog.module
shouldn't page level docblock always come first?

dblog.test
missing focblocks for the protected variables
missing docblock for function getInfo()

private functions shouldn't be shown.

filijonka’s picture

Status: Needs review » Needs work
jhodgdon’s picture

Status: Needs work » Needs review

RE #19... Not everything in that review is quite correct...

a) We don't put docblocks in for getInfo() in tests.

b) I'm not sure what you mean by "private functions shouldn't be shown".

c) When reviewing it's very helpful to point out specifics. So for instance if there are verbs in the wrong tense, please say which ones. There are exceptions to the policy. See http://drupal.org/node/1354 for details.

d) Some functions do *not* include params and return value... such as theme functions. See http://drupal.org/node/1354 for details.

So I think I'll set this back to Needs Review, and wait for specifics and correctlys.

filijonka’s picture

sorry for tryingto help

jhodgdon’s picture

filijonka: Sorry you took my hopefully-constructive suggestions as a request that you not help. Nothing could be further from the truth! We value all attempts to help, but at the same time, I needed to make sure that the suggestions you made were not acted on, in the cases where they weren't correct. Also, I wanted to offer some suggestions to you for the next time you attempt a review.

jhodgdon’s picture

Status: Needs review » Needs work

Sorry for the delay in getting this patch reviewed! I took a fresh look at the patch in #18 (just looking at the patch itself -- I didn't yet apply the patch to see if there were other things that needed fixing). I think there are a couple of things we should fix:

a) In the CSS files, I think we've been putting a blank line between the @file block and the beginning of the CSS.

*** dblog.admin.inc

b) The word entirety is misspelled here:

+ * Messages are truncated at 56 chars. Full-length message can be viewed in
+ * their entirity on the message details page (admin/reports/event/%).

Also, shouldn't it be "Full-length messages"?

c) Whenever you use "e.g.", it should be followed by a comma, as in:

+ * @param string $type
+ *   Type of database log events to display (e.g. 'search').

should be (e.g., 'search').

d)

 /**
- * List dblog administration filters that can be applied.
+ * Creates a list of database log administration filters that can be applied.
+ *
+ * @return array
+ *   Two-dimensional array with 'type' and 'severity' top-level keys, each of
+ *   which is an array with the following keys:
+ *   - title: Title of the filter.
+ *   - where: The filter condition.
+ *   - options: Types of watchdog messages or severity levels.
+ *   If no records exist in the table, the 'type' element will not be returned.
  */
 function dblog_filters() {+ * @return array
+ *   Two-dimensional array with 'type' and 'severity' top-level keys, each of
+ *   which is an array with the following keys:

I'm not happy with the @return section here:
- I believe all arrays in PHP are two-dimensional. I think you should just call it "array" or "associative array".
- The grammar here is misleading -- it says the *keys* are arrays. Should probably say "each of whose elements are arrays with the following keys". But actually, how about just leaving out the details of 'type' and 'severity -- since the function description is more generic (a list of filters that can be applied), can't the return value description be generic? So how about:
Associative array of filters. The top-level keys are used as the form element name for the filters, and the values are arrays with the following keys:

e)

 /**
- * Returns HTML for a log message.
+ * Returns an HTML string for a database log event.

Actually, the original was better than the patch. See
http://drupal.org/node/1354#themeable

f)

@@ -314,12 +334,13 @@ function dblog_filter_form($form) {
       '#value' => t('Reset')
     );
   }
-
   return $form;
 }

Why did this line get deleted? At a minimum it is out of scope for this issue... and I'm not sure our coding standards even suggest not having blank lines here.

g) The "word" dblog is used quite a few times in dblog.admin.inc documentation. Please replace with something that is actually a word, such as "database logging", or if it refers to the module, "the Database Logging module".

*** dblog.test ***

h) We aren't generally cleaning up in-code comments in the cleanup patches, but if you are going to fix this line:

-    // Fetch row ids in watchdog that relate to the user.
+    // Fetch the row ids in watchdog that relate to the user.

you could also fix "ids". "id" is a psychological term. Use "ID" for identifier, and its plural is "IDs".

batigolix’s picture

Assigned: Unassigned » batigolix

i ll give this a shot

batigolix’s picture

Status: Needs work » Needs review
FileSize
29.63 KB

attached patch incorporates the comments from #24

Status: Needs review » Needs work

The last submitted patch, cleanup_dblog_module_docs-1326600-26.patch, failed testing.

batigolix’s picture

cannot reroll the patch from #18

jhodgdon’s picture

What problem are you having rerolling the patch? See
http://drupal.org/patch/reroll

And if you no longer plan to work on this issue, please go ahead and un-assign it. Thanks for trying!

jhodgdon’s picture

Assigned: batigolix » Unassigned
batigolix’s picture

Assigned: Unassigned » batigolix

i followed patch-reroll instructions from xjm's blog. i ll try with the info in the link above

batigolix’s picture

Status: Needs work » Needs review
FileSize
28.94 KB

Here is a new patch that is a reroll of #18 and that incorporates the comments from #24.

jhodgdon’s picture

jhodgdon’s picture

Status: Needs review » Needs work

I'm sorry for the delay in reviewing this! The patch is mostly excellent, but I have a couple of concerns:

a) dblog_overview()

- * Messages are truncated at 56 chars. Full-length message could be viewed at
- * the message details page.
+ * Messages are truncated at 56 chars. Full-length messages can be viewed on
+ * the message details page (admin/reports/event/%)

We don't usually want to have specific paths mentioned in functions. They are always in flux and it gets hard to maintain them. Maybe an @see for the dblog_event() function would be better?

b) dblog_event()

+ * @param int $id
+ *   Unique watchdog event ID.

I don't think we should call this a "watchdog" ID, unless you are talking about a specific field of the {watchdog} table, in which case {watchdog} should be in {} followed by .fieldname.

c) dblog_filters() return value

+ *   Associative array of filters. The top-level keys are used as the form
+ *   element name for the filters, and the values are arrays with the following
+ *   keys:
+ *   - title: Title of the filter.
+ *   - where: The filter condition.
+ *   - options: Types of watchdog messages or severity levels.
+ *   If no records exist in the table, the 'type' element will not be returned.
  */
 function dblog_filters() {

- ...as the form element name... => names
- ...with the following keys: => elements
- I think on the 'options' key I would say "Array of options for the select list for the filter".
- That last line... "if no records exist in the table" -- that doesn't say what table it's talking about, and nowhere else does it say anything about what filters should normally be returned. Maybe it would be best just to leave this out... either that or to say something more about what filters should normally be returned?

d) dblog_clear_form()

+ * Form constructor for the dblog clear log form.

Should not say "dblog"... maybe something like "Form constructor for the form that clears out the log"?

e)

+++ b/core/modules/dblog/dblog.css
@@ -1,3 +1,8 @@
+
+/**
+ * @file
+ * Admin styles for the Database Logging module.
+ */

I don't think we want a blank line at the top of the file, and there should be one after the @file block. Applies to the RTL CSS file too.

f)

+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DBLogTest.php
@@ -25,7 +25,7 @@ class DBLogTest extends WebTestBase {
   }
 
   /**
-   * Enable modules and create users with specific permissions.
+   * Enables modules and creates users with specific permissions.
    */
   function setUp() {

At the moment, our standard is not to document test setUp() methods. Sigh.

g) Generally this cleanup is only supposed to do /** */ comments, not in-code // comments... I would prefer to have those left out of the patch (they are difficult to review). I definitely would *not* do changes like this in code at all:

-      'message'     => 'Log entry added to test the doClearTest clear down.',
+      'message'     => 'Log entry added to test the testDBLogAddAndClear clearing logic.',

h) This cleanup effort is also not supposed to be adding param/return types:

-   * @param $element
+   * @param SimpleXMLElement $element

Again, it makes the patch longer and more difficult to review.

batigolix’s picture

Status: Needs work » Needs review
FileSize
28.33 KB

Processed the feedback from #34

Regarding your remark:

Generally this cleanup is only supposed to do /** */ comments, not in-code // comments... I would prefer to have those left out of the patch (they are difficult to review).

There are many in-code comments in this patch. Do you prefer to have them removed?

Status: Needs review » Needs work

The last submitted patch, cleanup_dblog_module_docs-1326600-35.patch, failed testing.

batigolix’s picture

Status: Needs work » Needs review
FileSize
28.33 KB

new attempt

Status: Needs review » Needs work

The last submitted patch, cleanup_dblog_module_docs-1326600-37.patch, failed testing.

batigolix’s picture

Status: Needs work » Needs review
FileSize
27.38 KB

arr. new attempt

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! What is in the patch looks good, so I went ahead and committed it to 8.x.

There is one small follow-up needed: In the test file lib/Drupal/dblog/Tests/DBLogTest.php, the test class has no doc block.

batigolix’s picture

Added the doc block.

There is a spelling mistake elsewhere in the file (not in the api docs section):

verify user access to log reports based on persmissions

batigolix’s picture

Status: Needs work » Needs review

changed status

Cameron Tod’s picture

Status: Needs review » Needs work
+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DBLogTest.phpundefined
@@ -10,6 +10,9 @@ namespace Drupal\dblog\Tests;
+/**
+ * Tests logging messages in database
+ */

This is missing the definite article, and some punctuation. How about:

"Tests logging messages to the database."

Thanks!

batigolix’s picture

Status: Needs work » Needs review
FileSize
492 bytes

fixed

Cameron Tod’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, thanks mate.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x. Now I think we can port this entire thing to 7.x. Thanks!

Lars Toomre’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Needs review
FileSize
3.98 KB

I did a review of the complete Dblog module and must say that this is in the best shape yet from an API docs perspective. I found a couple of small items that are addressed in the attached patch. Hopefully, these can be added to D8 before the full backport to D7.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! This is mostly good cleanup... A couple of things:

a)

+ * @return array|string
+ *   If the $id is located in the Database Logging table, a build array in the
+ *   format expected by drupal_render(); otherwise, an empty string.
+ *
* @see dblog_menu()
  */
 function dblog_event($id) {

I wouldn't say "the $id". Either "the ID" or just "$id".

b)

+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DBLogTest.php
+  /**
+   * The installation to use with this test.
+   *
+   * @var string
+   */
   protected $profile = 'standard';

Should be "installation *profile* to use...".

Other than that, looks great, thanks!

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
4.48 KB

In the last week, the member $profile was eliminated from DBLogTest.php, hence #47 needed to be re-rolled anyways. This locally untested patch addresses the point in 48 a) by using 'the ID'.

Status: Needs review » Needs work

The last submitted patch, 1326600-49-dblog-docs.patch, failed testing.

Lars Toomre’s picture

I think the fail in #49 is unrelated to this patch since it is occurring in UserAccccountLinksTest. Let's try the bot again.

Lars Toomre’s picture

Status: Needs work » Needs review

#49: 1326600-49-dblog-docs.patch queued for re-testing.

jhodgdon’s picture

Assigned: batigolix » jhodgdon
Status: Needs review » Reviewed & tested by the community

This looks good, thanks! I'll get it committed shortly (hope it still applies).

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! That last patch is in 8.x now. We have several patches to port to d7 as much as possible (they can be combined).

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
28.16 KB

Backported #'s 39, 44, and 49 to D7.

Status: Needs review » Needs work
Issue tags: -Novice, -Needs backport to D7, -docs-cleanup-2011

The last submitted patch, 1326600-55-dblog-docs-cleanup.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs backport to D7, +docs-cleanup-2011
jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Thank you! This looks fine -- I'll get it committed in the next few days.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks! Committed this patch to 7.x and this issue is DONE! If you have follow-ups, at this point open another issue.

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

Anonymous’s picture

Issue summary: View changes

Added follow-up issues header to summary.