API page: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

Enter a descriptive title (above) relating to Condition.php, then describe the problem you have found:

This class file has no documentation at the top of the class. Not even one line. Needs some.

This is a clear violation of the Core Gates for Documentation, so it is at least a major bug. Should be easy to fix -- there is a very similar class just one directory up.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Also I think there are some old "Implements..." documented methods in this file that should be converted to inheritdoc.

divesh.kumar’s picture

FileSize
1.39 KB

I have changed the documentation for it.

longwave’s picture

Status: Active » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patch!

A class (or anything else) needs a one-line summary to start its documentation.

+/**
+ * Extending a class from ConditionBase to implement condition clause existance
+ * checking in SQL queries.
+ */
 class Condition extends ConditionBase {

And for a class, this should start with a verb. We have standards on how to write documentation -- see
https://drupal.org/coding-standards/docs#classes
and this general section:
https://drupal.org/coding-standards/docs#drupal

The rest of the patch looks good.

divesh.kumar’s picture

Status: Needs work » Needs review
FileSize
1.87 KB

I have made changes as suggested.

divesh.kumar’s picture

FileSize
1.15 KB
jhodgdon’s picture

Status: Needs review » Needs work

Oops, the one-line documentation intended for the class got into a method instead.

I also have a suggestion for the one-line class documentation... How about:

Implements entity query conditions for SQL databases.

(This is a class specifically for entity query conditions, and I think we need to get that into the documentation.)

divesh.kumar’s picture

Status: Needs work » Needs review
FileSize
2.81 KB

My bad!!! Made changes in the new patch..

Status: Needs review » Needs work

The last submitted patch, 8: 2191721-3.patch, failed testing.

divesh.kumar’s picture

Status: Needs work » Needs review
FileSize
1.33 KB

Status: Needs review » Needs work

The last submitted patch, 10: 2191721-4.patch, failed testing.

divesh.kumar’s picture

Patch is failing due to LocalePluralFormatTest.php.

longwave’s picture

Status: Needs work » Needs review

10: 2191721-4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2191721-4.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

Thanks! Test failure is unrelated, and I don't think we need to hit "retest" as it's some "table not found" glitch in the test bot on one test. The patch looks good.

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.