Support from Acquia helps fund testing for Drupal Acquia logo

Comments

duellj’s picture

Status: Active » Needs review
FileSize
44.88 KB

Converted taxonomy term and vocabulary classes over to PSR-0 and updated docblocks.

Status: Needs review » Needs work

The last submitted patch, taxonomy-psr-1533022-1.patch, failed testing.

duellj’s picture

Status: Needs work » Needs review
FileSize
44.99 KB

This should fix the failing forum test

Berdir’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Term.phpundefined
@@ -0,0 +1,96 @@
+<?php
+
+/**
+ * @file
+ * Entity class Taxonomy term.
+ */
+
+namespace Drupal\taxonomy;
+
+use Entity;
+
+/**
+ * Defines the taxonomy term entity.
+ */
+class Term extends Entity {

The entity type is still called taxonomy_term, I'm not sure if we should really rename it the class to Term. I'm fine with it if others are, just wanted to point it out.

duellj’s picture

I was going off of fago's lead in http://drupal.org/node/1495024#comment-5855622

Since Term and Vocabulary classes are already namespaced to Drupal\taxonomy, I don't think it makes sense to repeat the module in the class name again.

RobLoach’s picture

#3: taxonomy-psr-1533022-3.patch queued for re-testing.

fago’s picture

Since Term and Vocabulary classes are already namespaced to Drupal\taxonomy, I don't think it makes sense to repeat the module in the class name again.

That was my thought to - I see no need to double the module prefix.

Status: Needs review » Needs work

The last submitted patch, taxonomy-psr-1533022-3.patch, failed testing.

xjm’s picture

Issue tags: +Needs change record

We should also add information about this conversion to the original change notification at http://drupal.org/node/1400186 http://drupal.org/node/1479568 once it is ready.

duellj’s picture

Status: Needs work » Needs review
FileSize
44.97 KB

Rerolled against HEAD, should apply cleanly now.

duellj’s picture

Noticed that api docs should use full namespaces for type hinting (http://drupal.org/node/1353118). Updated taxonomy.api.php accordingly.

Berdir’s picture

I find that api.php related block there quite strange and contradicting with the next sentence that requires "use" when you use it multiple times. But that's nothing to be discussed here.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermController.phpundefined
@@ -0,0 +1,146 @@
+/**
+ * Controller class for taxonomy terms.
+ */
+class TermController extends EntityDatabaseStorageController {

Wondering if we should change this to TermStorageController while we're at it, all other classes have been renamed like that already, have a look at http://drupal.org/node/1400186.

Also, that change record needs to be updated to use the namespace like user/node now do.

aspilicious’s picture

I vote for renaming now :).
And yeah I don't think we agreed to still use "use" when using it multiple times in api documentation...

Crell’s picture

The current coding standard detente was:

1) api.php files should use full class names in hooks, so that they are copy-pasteable.
2) If you use a class exactly once in a file in a function/method signature, you may leave it long-form. (ie, if you just copied from the api.php file.)
3) In all other cases "use" the class.

That lone exception was made for copy-paste-ability of example hooks basically.

aspilicious’s picture

Ok I'm going to correct the api docs to reflect we always have to use the full name in hook documentation.

duellj’s picture

Renamed TermController to TermStorageController and VocabularyController to VocabularyStorageController (from #12).

Looks like there's nothing to correct in taxonomy.api.php, correct?

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/entity/entity.controller.incundefined
@@ -248,7 +248,7 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
    * into terms, however it can also support $conditions on different tables.
-   * See CommentController::buildQuery() or TaxonomyTermController::buildQuery()
+   * See CommentController::buildQuery() or TermStorageController::buildQuery()

This needs to be the fully qualified name, no?

RTBC to me except of that reference there.

xjm’s picture

Issue tags: +Novice

NW also means I can point out the docs issues that have been bothering me with this patch. I realize some of the issues below probably go back to the initial conversion. Tagging novice for some cleanup.

  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Term.phpundefined
    @@ -0,0 +1,96 @@
    + * @file
    + * Entity class Taxonomy term.
    
    +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Vocabulary.phpundefined
    @@ -0,0 +1,70 @@
    +/**
    + * @file
    + * Entity class Taxonomy vocabulary.
    + */
    

    From http://drupal.org/node/1354#classes:

    For files containing the definition of exactly one class, use this template:

    /**
     * @file
     * Definition of NameOfTheClass.
     */
    
  2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Term.phpundefined
    @@ -0,0 +1,96 @@
    +   * (optional) Description of the term.
    

    Are we documenting other optional member variables this way in the other conversions? I guess it's a natural extension of our @param documentation, but I doubt the API module supports it (yet?). Reference: http://drupal.org/node/1354#classes

  3. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Term.phpundefined
    @@ -0,0 +1,96 @@
    +   * (optional) The machine name of the vocabulary the term is assigned to. If
    +   * not given, this value will be set automatically by loading the vocabulary
    +   * based on the $entity->vid property.
    

    This should be broken into two paragraphs, with the summary as just the first sentence here. Reference: http://drupal.org/node/1354#doxygen-general

  4. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
    @@ -0,0 +1,146 @@
    + * @file
    + * Entity controller class for Taxonomy term.
    
    +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyStorageController.phpundefined
    @@ -0,0 +1,89 @@
    +/**
    + * @file
    + * Entity controller class for Taxonomy vocabulary.
    + */
    

    From http://drupal.org/node/1354#classes:

    For files containing the definition of exactly one class, use this template:

    /**
     * @file
     * Definition of NameOfTheClass.
     */
    
  5. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
    @@ -0,0 +1,146 @@
    +/**
    + * Controller class for taxonomy terms.
    + */
    
    +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyStorageController.phpundefined
    @@ -0,0 +1,89 @@
    +/**
    + * Controller class for taxonomy vocabularies.
    + */
    

    These docblocks should begin with a third-person verb. Reference: http://drupal.org/node/1354#classes

  6. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
    @@ -0,0 +1,146 @@
    +    // Ensure the vocabulary machine name is initialized as it is used as bundle
    +    // key.
    

    This sentence needs an article: "the bundle key."

  7. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
    @@ -0,0 +1,146 @@
    +    // @todo Move to Term::bundle() once field API has been converted
    +    // to make use of it.
    

    The second line of the @todo should be indented two spaces. Reference: http://drupal.org/node/1354#todo

xjm’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Term.phpundefined
@@ -0,0 +1,96 @@
+   * (optional) The weight of this term in relation to other terms of the same
+   * vocabulary.

Ooops, one more too-long summary. :)

duellj’s picture

Fixed issues from #18 and #19.

Are we documenting other optional member variables this way in the other conversions? I guess it's a natural extension of our @param documentation, but I doubt the API module supports it (yet?).

It looks like the (optional) convention is supported (though I don't know if by the API module): http://drupal.org/node/1354#doxygen-general

+++ b/core/modules/entity/entity.controller.incundefined
@@ -248,7 +248,7 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
* into terms, however it can also support $conditions on different tables.
- * See CommentController::buildQuery() or TaxonomyTermController::buildQuery()
+ * See CommentController::buildQuery() or TermStorageController::buildQuery()

This needs to be the fully qualified name, no?

Updated both of these references (CommentController should be CommentStorageController). If that's outside the scope of this issue, I can revert it.

duellj’s picture

Status: Needs work » Needs review
xjm’s picture

Thanks for the cleanups!

It looks like the (optional) convention is supported (though I don't know if by the API module): http://drupal.org/node/1354#doxygen-general

Right, but that's only for @param that I've ever seen. I've never seen it for @var before.

aspilicious’s picture

Without the "optional"

Berdir’s picture

#23: taxonomy-psr-1533022-23.patch queued for re-testing.

klausi’s picture

Status: Needs review » Needs work
+++ b/core/modules/taxonomy/taxonomy.module
@@ -989,7 +992,7 @@ function taxonomy_vocabulary_load_multiple($vids = array(), array $conditions =
- * @return TaxonomyVocabulary|false
+ * @return Vocabulary|false

should be fully namespaced, right?

>$ grep -rin "@return Term" *
core/modules/taxonomy/taxonomy.module:1028: * @return Term|false
>$ grep -rin "@return Vocabulary" *
core/modules/taxonomy/taxonomy.module:995: * @return Vocabulary|false
core/modules/taxonomy/taxonomy.module:1011: * @return Vocabulary|false
aspilicious’s picture

Status: Needs work » Needs review
FileSize
46.1 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, taxonomy-psr-1533022-26.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
46.14 KB

Stupid namespaces ;)

Berdir’s picture

Status: Needs review » Needs work
$ grep -R "TaxonomyTerm" core/ | grep -v ".test"
core/modules/entity/lib/Drupal/entity/EntityController.php:   * See CommentController::buildQuery() or TaxonomyTermController::buildQuery()

Looks like the same as in #17, did we loose that change somehow?

aspilicious’s picture

Status: Needs work » Needs review
FileSize
47.12 KB

Next try

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

All renames look good, the remarks from @xjm in #18 have been addressed. I think this is RTBC. Remember to update http://drupal.org/node/1400186, the controller classes are currently completely missing there.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Novice, -Needs change record