Problem/Motivation

The docs say:

   * @param array $components
   *   (optional) Array of component lists indexed by type. If not present or it
   *   is an empty array, it will update all components.

'update' is wrong surely, as everything else about the method says it just returns things.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3422919

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

joachim created an issue. See original summary.

Prem Suthar made their first commit to this issue’s fork.

Prem Suthar opened merge request !6723

joachim’s picture

joachim commented
Status: Active » Needs work

Thanks for the MR!

Could you just fix the one problem for this issue though, rather than expand the scope?

I am going to be filing an issue to fix the confusing use of 'component' to mean extensions, but it's going to involve method renaming too.

prem suthar’s picture

prem suthar commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

@Prem Suthar just FYI this was tagged for novice, meaning for new users to Drupal contribution, and based on your post history believe you could work on non novice issues please.

Believe the goal was just to update getComponentNames function.

joachim’s picture

joachim commented

I think we could expand the scope to include the other error that @Prem Suthar spotted, which is that the structure of the @param array is incorrectly documented:

> Array of component lists indexed by type

However, the current MR is removing the '(optional)' part.

I've filed #3422955: Fix incorrect use of word 'component' in locale module, which is a bigger task which covers both code and docs.

smustgrave’s picture

smustgrave commented
Issue tags: +Needs issue summary update

Sure but should be documented

joachim’s picture

joachim commented

Hmm on second thoughts,

> Array of component lists indexed by type

does sort of describe it -- it's an array of several lists, and the array is keyed by the type of the things in the list.

The other issue is going to rewrite that text anyway so let's just fix the incorrect 'update' here.

lee jun long’s picture

lee jun long commented

Hi, I am looking into this issue queue as part of Drupalcon Singapore 2024 Contribution day, and hope I can contribute in one way or another.

I have made a fresh installation of a Drupal 11 site to check on this, and tried to search the codebase for the code block shown in the Problem/Motivation. I realised that the only location I can find comes from core/modules/locale/src/LocaleConfigManager.php. I checked that merge request MR !6723, and found that the MR includes changes to both core/modules/locale/src/LocaleConfigManager.php and core/modules/locale/locale.bulk.inc. This confused me as I didn't understand the current state of the issue queue, and how I can contribute further.

joachim’s picture

joachim commented

It's core/modules/locale/src/LocaleConfigManager.php that needs fixing.

adwivedi008 changed the visibility of the branch 3422919-error-in-docs to hidden.

adwivedi008 changed the visibility of the branch 3422919-error-in-docs to active.

kul.pratap’s picture

kul.pratap commented

Working on this.

adwivedi008 changed the visibility of the branch 3422919-error-in-docs to hidden.

adwivedi008 changed the visibility of the branch 3422919-error-in-docs to hidden.

adwivedi008 changed the visibility of the branch 3422919-error-in-docs to active.

adwivedi008 changed the visibility of the branch 3422919-error-in-docs to hidden.

adwivedi008 changed the visibility of the branch 3422919-error-in-docs to active.

adwivedi008’s picture

adwivedi008 commented
Status: Needs work » Needs review

Reverted the out-of-scope changes.
please review

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Issue summary for good practice should still be filled out.

Also the parameter is still optional.

brandonlira’s picture

brandonlira commented

Error in docs for getComponentNames()

Problem/Motivation

The docs say: 

@param array $components
  (optional) Array of component lists indexed by type. If not present or it
  is an empty array, it will update all components.

The use of the word "update" is incorrect, as the method actually returns configuration names rather than updating anything.

Steps to reproduce

  1. Open core/modules/locale/src/LocaleConfigManager.php.
  2. Locate the getComponentNames() method.
  3. Check the docblock and see that it mentions “update” when the method is only returning configuration names.

Proposed resolution

  • Remove the word “update” from the docblock.
  • Clarify that if $components is empty or not provided, the method returns all configuration names.
  • Retain that the parameter is (optional).

Remaining tasks

  • Update the docblock and push changes to the issue fork.
  • Review and test the updated documentation.
  • Await merge request approval.

User interface changes

None.

API changes

None; this is purely a documentation fix.

Data model changes

None.

Release notes snippet

Not applicable.

brandonlira’s picture

brandonlira commented
Status: Needs work » Needs review
brandonlira’s picture

brandonlira commented

Hi everyone,

I've updated the issue summary in my comment (#22) with the correct documentation details for the getComponentNames() method. It seems I don't have permission to edit the main issue summary directly. Could someone with the necessary permissions please update the issue summary with my changes?

Thank you!

joachim’s picture

joachim commented
Status: Needs review » Needs work
brandonlira’s picture

brandonlira commented
Status: Needs work » Needs review

I have pushed my changes for Issue #3422919, which restore the "(optional)" note in the docblock of the getComponentNames() method. Please review my changes in MR and let me know if further adjustments are needed.

joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

Looks good. Thanks!

nod_ made their first commit to this issue’s fork.

nod_ closed merge request !6723

  • nod_ committed e7a6df2a on 11.x 
    Issue #3422919 by prem suthar, adwivedi008, brandonlira, joachim,...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: error in docs for getComponentNames() » Fix documentation of LocaleConfigManager::getComponentNames()
Status: Reviewed & tested by the community » Fixed

Committed e7a6df2 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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