Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Issue tags: +PSR-0

Adding tag

duellj’s picture

Status: Active » Needs review
FileSize
27.81 KB

Ok, here's the first attempt at converting Comment and CommentStorageController to PSR-0. Also changed all docblock references to Comment to the fully qualified name.

aspilicious’s picture

Are these all the instances of "Comment" in core?

duellj’s picture

FileSize
28.41 KB

Good question, did another pass and I missed a couple of docblock updates in comment.test. But other than that I think I caught all instances of "Comment" in core

RobLoach’s picture

#4: comment-psr-1533020-4.patch queued for re-testing.

xjm’s picture

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.

aspilicious’s picture

If I'm correct, we should use the full namespace path in api.php files. That way we can easily copy paste example code if needed:

+ * @param Drupal\comment\Comment $comment
  *   The comment object.
  */
 function hook_comment_presave(Comment $comment) {

should be

+ * @param Drupal\comment\Comment $comment
  *   The comment object.
  */
 function hook_comment_presave(Drupal\comment\Comment $comment) {

Look at http://drupal.org/node/1353118 for more info

Example: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

Berdir’s picture

Issue tags: -PSR-0

#4: comment-psr-1533020-4.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +PSR-0

The last submitted patch, comment-psr-1533020-4.patch, failed testing.

duellj’s picture

Status: Needs work » Needs review
FileSize
30.64 KB

Rerolled patch and updated api docs to include full namespaces (from #7).

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

catch’s picture

Title: Convert comment.module entity classes to PSR-0 » Change notification for: Convert comment.module entity classes to PSR-0
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Thanks! Committed/pushed to 8.x.

aspilicious’s picture

Status: Active » Needs review
Berdir’s picture

Looks good. @xjm also mentioned http://drupal.org/node/1479568, but I have no clue how to add it there. The only thing that has been converted is the CommentController class.

aspilicious’s picture

We have a subsystem => namespace table.
What if we add a module => namespace table under it?

Berdir’s picture

and list what there? That comment.module is now Drupal\comment? That's kinda obvious and can be listed in a single sentence for all modules, no?

For it to make sense, we'd need to actually list single classes there. Which would be possible because there aren't that many non-test classes in modules that already existed in 7.x.

xjm’s picture

I think just in the entity one was fine. Someone convinced me it should go in the PSR-0 one, but I think so long as we just make sure the issue is referenced on both, what Berdir and aspilicious have already added is quite clear.

aspilicious’s picture

Status: Needs review » Fixed

Ok references are added. Ow yeah! Marking this fixed!

Tor Arne Thune’s picture

Title: Change notification for: Convert comment.module entity classes to PSR-0 » Convert comment.module entity classes to PSR-0
Priority: Critical » Normal

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