Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#30 | taxonomy-psr-1533022-30.patch | 47.12 KB | aspilicious |
#28 | taxonomy-psr-1533022-28.patch | 46.14 KB | aspilicious |
#26 | taxonomy-psr-1533022-26.patch | 46.1 KB | aspilicious |
#23 | taxonomy-psr-1533022-23.patch | 46.75 KB | aspilicious |
#20 | taxonomy-psr-1533022-20.patch | 45.3 KB | duellj |
Comments
Comment #1
duellj CreditAttribution: duellj commentedConverted taxonomy term and vocabulary classes over to PSR-0 and updated docblocks.
Comment #3
duellj CreditAttribution: duellj commentedThis should fix the failing forum test
Comment #4
BerdirThe 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.
Comment #5
duellj CreditAttribution: duellj commentedI 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.
Comment #6
RobLoach#3: taxonomy-psr-1533022-3.patch queued for re-testing.
Comment #7
fagoThat was my thought to - I see no need to double the module prefix.
Comment #9
xjmWe should also add information about this conversion to the original change notification at
http://drupal.org/node/1400186http://drupal.org/node/1479568 once it is ready.Comment #10
duellj CreditAttribution: duellj commentedRerolled against HEAD, should apply cleanly now.
Comment #11
duellj CreditAttribution: duellj commentedNoticed that api docs should use full namespaces for type hinting (http://drupal.org/node/1353118). Updated taxonomy.api.php accordingly.
Comment #12
BerdirI 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.
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.
Comment #13
aspilicious CreditAttribution: aspilicious commentedI vote for renaming now :).
And yeah I don't think we agreed to still use "use" when using it multiple times in api documentation...
Comment #14
Crell CreditAttribution: Crell commentedThe 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.
Comment #15
aspilicious CreditAttribution: aspilicious commentedOk I'm going to correct the api docs to reflect we always have to use the full name in hook documentation.
Comment #16
duellj CreditAttribution: duellj commentedRenamed TermController to TermStorageController and VocabularyController to VocabularyStorageController (from #12).
Looks like there's nothing to correct in taxonomy.api.php, correct?
Comment #17
BerdirThis needs to be the fully qualified name, no?
RTBC to me except of that reference there.
Comment #18
xjmNW 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.
From http://drupal.org/node/1354#classes:
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
This should be broken into two paragraphs, with the summary as just the first sentence here. Reference: http://drupal.org/node/1354#doxygen-general
From http://drupal.org/node/1354#classes:
These docblocks should begin with a third-person verb. Reference: http://drupal.org/node/1354#classes
This sentence needs an article: "the bundle key."
The second line of the @todo should be indented two spaces. Reference: http://drupal.org/node/1354#todo
Comment #19
xjmOoops, one more too-long summary. :)
Comment #20
duellj CreditAttribution: duellj commentedFixed issues from #18 and #19.
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
Updated both of these references (CommentController should be CommentStorageController). If that's outside the scope of this issue, I can revert it.
Comment #21
duellj CreditAttribution: duellj commentedComment #22
xjmThanks for the cleanups!
Right, but that's only for
@param
that I've ever seen. I've never seen it for@var
before.Comment #23
aspilicious CreditAttribution: aspilicious commentedWithout the "optional"
Comment #24
Berdir#23: taxonomy-psr-1533022-23.patch queued for re-testing.
Comment #25
klausishould be fully namespaced, right?
Comment #26
aspilicious CreditAttribution: aspilicious commentedRerolled
Comment #28
aspilicious CreditAttribution: aspilicious commentedStupid namespaces ;)
Comment #29
BerdirLooks like the same as in #17, did we loose that change somehow?
Comment #30
aspilicious CreditAttribution: aspilicious commentedNext try
Comment #31
BerdirAll 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.
Comment #32
Dries CreditAttribution: Dries commentedCommitted to 8.x.
Comment #34
xjm