Problem/Motivation

Several methods on EntityDisplayRepositoryInterface - all the get*Modes() - have @return docs that say something like this:

   * @return array
   *   The view mode info for all entity types.

What is the 'info'? What is the array keyed by? The docs should say this.

Steps to reproduce

Proposed resolution

Complete the documentation.

It's probably fine for the entity type-specific flavours to refer to the general method for docs, to avoid repetition.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3411185

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.

joachim’s picture

joachim commented

This should also say that the 'default' view mode is not returned -- using the return of one of these to iterate through getViewDisplay() for all modes will omit it!

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

govind_giri_goswami changed the visibility of the branch 3411186- to hidden.

govind_giri_goswami changed the visibility of the branch 3411185- to hidden.

govind_giri_goswami changed the visibility of the branch 3411185-docs-for-return to hidden.

govind_giri_goswami opened merge request !6469

govind_giri_goswami’s picture

govind_giri_goswami commented
Status: Active » Needs review

Each method (e.g., getAllViewModes(), getViewModes(), getAllFormModes(), getFormModes()) is now individually documented. Previously, their return documentation simply stated @return array, lacking clarity on the array's structure. The updated documentation provides additional details for clarity, enhancing developers' understanding.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I think this is mostly better than what we had in the original docblocks, but the wording here is not something we typically use in Core. I think we should only include the ones we know for sure that exist.

Can we find documentation that is similar and copy the style?
Not sure what @joachim thinks about this

joachim’s picture

joachim commented
Status: Needs review » Needs work

> I think we should only include the ones we know for sure that exist.

Agreed.

If other keys might exist, we can say that, for instance (totally making this up) 'Further properties may be added by modules that implement hook_foo_alter()'.

If the return of all of these has the same format, I think it's ok to use a @see getAllViewModes() and only have the docs in one place.

Also see #2.

akshaydalvi212’s picture

akshaydalvi212 commented
Assigned: Unassigned » akshaydalvi212

Will provide some docs for the returns values.

akshaydalvi212’s picture

akshaydalvi212 commented
Assigned: akshaydalvi212 » Unassigned
rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented
Issue tags: +Barcelona2024

The Drupal Contribution Mentoring team is triaging issues for DrupalCon Barcelona 2024, and we are reserving this issue for Mentored Contribution during the event.

After September 27, 2024, this issue returns to being open to all. Thanks!

By looking at the failed tests I think there is typo "Initail"

santhosh@21’s picture

santhosh@21 commented

I am working on this issue

santhoshkumar Gurram opened merge request !9637

santhosh@21’s picture

santhosh@21 commented

I have updated the spellcheck and the pipeline is failing in different functional tests related to the Umami and that doesn't suits for this document return type issue.

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented
Status: Needs work » Needs review

Did a rebase and the pipeline is green again

sadamafridi’s picture

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

I have reviewed the fix, the pipeline is green and spell mistake has been corrected.

santhosh@21’s picture

santhosh@21 commented

Thank you @sadamafridi and @rodrigoaguilera for reviewing the issue and updating the comments

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

Thanks for working on this!

The issue summary and title state this is for EntityDisplayRepositoryInterface() and yet there are changes to several files in the composer directory. Those are out of scope and should be moved to their own issue.

Thanks

quietone’s picture

quietone commented

I should have added that we do have guidelines for scoping of an issue, Issue scope guidelines for Drupal core issues. They explain why we limit the scope of an issue.

brandonlira’s picture

brandonlira commented
Assigned: Unassigned » brandonlira
brandonlira’s picture

brandonlira commented
Status: Needs work » Needs review

Hi everyone,

I have updated the merge request to remove unrelated changes in the composer/ directory and keep only the documentation improvements for EntityDisplayRepositoryInterface.php.

Now, the MR focuses solely on clarifying the return values for the getAllViewModes(), getViewModes(), getAllFormModes(), and getFormModes() methods, following the previous feedback.

Please review the changes and let me know if any further adjustments are needed.

Thanks!

brandonlira’s picture

brandonlira commented
Assigned: brandonlira » Unassigned
brandonlira’s picture

brandonlira commented
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Don't see any commit for #23

brandonlira’s picture

brandonlira commented

Hi @smustgrave ,

I can confirm that my changes are included in the latest pipeline (#449876), even though the commit history might not explicitly show my name due to the rebase process.

The updates focus solely on documentation improvements for EntityDisplayRepositoryInterface.php MR !9637 , and all unrelated changes have been removed.

Please review and let me know if any further adjustments are needed.

Thanks!

brandonlira’s picture

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

smustgrave commented
Status: Needs review » Needs work

I don’t believe that’s true the MR is 700+ commits behind

brandonlira’s picture

brandonlira commented
Status: Needs work » Needs review

Hi @smustgrave,

I have successfully rebased the branch with the latest changes from 11.x. The MR is now up to date, and the documentation improvements remain intact.

Additionally, I have fixed the PHPCS violations by adjusting the line lengths to conform with Drupal coding standards.

I also noticed that some tests have failed (`Drupal\Tests\package_manager\Build\PackageInstallTest` and `Drupal\Tests\ckeditor5\FunctionalJavascript\ImageUrlProviderTest`), but they do not seem to be related to the changes made in this MR.

Let me know if any further adjustments are needed.

Thanks!

smustgrave closed merge request !6469

smustgrave’s picture

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

Believe this one is good, reads fine to me

quietone’s picture

quietone commented

I updated credit.

  • quietone committed c85acd9a on 11.1.x 
    Issue #3411185 by govind_giri_goswami, brandonlira, santhosh@21,...

  • quietone committed 43f674f9 on 11.x 
    Issue #3411185 by govind_giri_goswami, brandonlira, santhosh@21,...

quietone closed merge request !9637

quietone’s picture

quietone commented
Version: 11.x-dev » 11.1.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks for improving documentation.

Pushed to 11.x and 11.1.x

Status: Fixed » Closed (fixed)

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