API page: https://api.drupal.org/api/drupal/core%21modules%21hal%21hal.install/8.3.x

This file is defining a topic for updates-8.2.x-to-8.3.x using a @defgroup documentation block.

However, this topic/group has already been defined in serialization.install. It should only be defined once. Subsequent uses should be using @addtogroup.

See
https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...
for documentation on how these tags work.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon created an issue. See original summary.

danielnv18’s picture

danielnv18’s picture

Version: 8.3.x-dev » 8.4.x-dev
Assigned: Unassigned » danielnv18
Status: Active » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Please read the documentation linked in the summary for @addtogroup. This isn't quite right. Thanks!

chiranjeeb2410’s picture

@danielnv18,

We only define each group identifier once with a @defgroup tag. To add more functions to the group present in a separate file or location, use either @addtogroup or @ingroup.

jhodgdon’s picture

@chiranjeeb2410 -- the patch already does that. The problem is the formatting of the @addtogroup tag. This is why I asked @danilenv18 to please read the documentation, which has an example of the formatting.

danielnv18’s picture

@jhodgdon thanks for the review. If I understood the documentation correctly this patch should do the work

<?php
**
 * @addtogroup group_identifier
 * @{
 */

(functions, classes, etc. that belong as members of the group go here)

/**
 * @} End of "addtogroup group_identifier".
 */
?>
danielnv18’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

yes, that's it exactly! Thanks!

alexpott’s picture

Issue tags: +Needs followup

@jhodgdon isn't there are bigger issue of where these update groups should be defined - like the module that adds the first update hook doesn't seem right? I think before we should have an issue to go through an check the update hooks and defgroups addtogroup stuff and come up with a procedure to add new groups as we open new branches.

chiranjeeb2410’s picture

FileSize
335 bytes

@danielnv18,

+1 on #7. Patch applies cleanly. Uploading interdiff, just to make things clear.

danielnv18’s picture

I changed the Drupal version from 8.3.x to 8.4.x since Drupal 8.3 is in RC phase but I might be wrong

jhodgdon’s picture

@alexpott: There is a question in my mind whether there even needs to be a group/topic for these. A group should only be defined if people would really want to look at a page that lists all the grouped functions. Is that really the case for update functions at all? You can find them easily by a search, such as:
https://api.drupal.org/api/drupal/8.3.x/search/_update_82

And by the way, if you click through to
https://api.drupal.org/api/drupal/core%21modules%21path%21path.install/f...
you will notice that there is no topic listed there. The reason is that path.install used
@addtogroup updates-8.2.0
whereas the defgroup was given the ID updates-8.1.x-to-8.2.x

So. This illustrates the problem. To me, it seems like we should just take out all the @defgroup and @addtogroup for update functions entirely, and let people search by function name, which is more reliable as far as finding the actual functions for a particular version. Plus, easier to maintain. That would be my suggestion.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs followup

@jhodgdon that sounds really sensible - far less to get wrong. Let's fix this here because we're unlikely to get this agreed for 8.3.0. I've filed #2860096: Remove api doc groups for updates eg. updates-8.2.x-to-8.3.x.

Committed and pushed 983612d to 8.4.x and ca17ab8 to 8.3.x. Thanks!

  • alexpott committed 983612d on 8.4.x
    Issue #2858598 by danielnv18, chiranjeeb2410, jhodgdon: hal.install has...

  • alexpott committed ca17ab8 on 8.3.x
    Issue #2858598 by danielnv18, chiranjeeb2410, jhodgdon: hal.install has...

Status: Fixed » Closed (fixed)

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