Problem/Motivation

Follow up to #2476947: Convert "title" page element into a block, we had to namespace views_add_contextual_links() in ViewPageControllerTest which was a copy of the same thing done in ViewBlockTest. In both cases the function needs to exist to prevent a fatal "function does not exist" error. If there is any potential for the function to be called, we need the functionality mocked, as well.

Also replace views_add_contextual_links() in views.module with a service and update all views_add_contextual_links() instances in core .

Proposed resolution

Convert views_add_contextual_links() to service and deprecate it.

Remaining tasks

None.

User interface changes

None.

API changes

  • views_add_contextual_links() is deprecated
  • New service Drupal\views\ContextualLinksHelper

Data model changes

None.

CommentFileSizeAuthor
#29 2571679-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-2571679

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

davidhernandez created an issue. See original summary.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Version: 9.5.x-dev » 11.x-dev
Status: Active » Needs work

narendraR opened merge request !4577

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs work » Needs review
Issue tags: +Needs change record

I will add the Draft CR once this MR is approved.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs title update, +Needs Review Queue Initiative

Think this could use an IS and title update. Seems to talk about page controller and block tests

Left a question on the MR

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Title: Replace views_add_contextual_links() in Views page controller and block tests » Replace views_add_contextual_links() in views.module with service
Issue summary: View changes
narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs title update
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Did a quick service for views_add_contextual_links and seems they were all replaced.

Only moving to NW for the change record to announce the deprecation.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs work » Needs review
Issue tags: -Needs change record

CR added.

smustgrave’s picture

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

Thanks!

quietone’s picture

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

I'm triaging RTBC issues. I read the IS, the comments and the CR. Nice to see BC considered and requests for the IS and title to be updated. I didn't find any unanswered questions. And the CR is brief and clear.

It is good to see all the documentation added to the new service. Thanks!

However, many of the comments both the new ones and the ones copied from existing code to the service are not wrapped at 80 columns. And there are some other changes needed for coding standards. I also had trouble following the long sentences is some of the paragraphs so am hoping that can be changed to make it easier for the reader.

Setting to needs work for the comments.

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

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs work » Needs review

Changes done as suggested.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left a comment on the one thread.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs work » Needs review
smustgrave’s picture

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

Believe this is ready.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 2571679-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

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

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Status: Needs work » Needs review

Resolved conflicts and rebased the MR.
Thanks!

smustgrave’s picture

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

Reroll seems fine.

quietone’s picture

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

I'm triaging RTBC issues. I read the IS and the comments.

I read the MR and left comments. Since there is work to do I am changing the status. For me, the key thing is to improve the docblock for \Drupal\views\ContextualLinks::addLinks. Oh, and to add return types.

Cheers

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

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Added a few comments. this has lots of conflicts with a recently committed issue and will need to be rebased.

claudiu.cristea made their first commit to this issue’s fork.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review

I've tried to rewrite the huge docblock but I would leave it to a native English speaker.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
Parent issue: » #3566536: [meta] eliminate core .module files

So far...

  • Rebased. Fixed conflicts
  • Addressed the review remarks
  • Updated deprecation messages
  • Replaced new service ID with its class FQN
  • Modernized the code moved to the new service
    • Added strict typing
    • Reduced cyclomatic complexity
    • Simplification
  • Update the method's docblock to make more sense
  • Updated the IS
  • Updated the CR

Ready for review

nicxvan’s picture

nicxvan commented

I really wonder about updating the doc block too much. Many times that gets pushed back, should we do a follow up?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

@nicxvan, I think is readable now. We can keep it here

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

I read through the comment and I have one really minor suggestion.

There is also a conflict, setting it to needs work for the conflict more than my suggestion.

Let me know when is rebased and I'll look again, but I think it's ready.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review

Fixed the conflict, applied suggestion. Ready for a new round of review

nicxvan’s picture

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

Noticed one final issue in the comment which I self applied, it was super minor.

I rebased so I could use git diff main --color-moved=dimmed_zebra --color-moved-ws=ignore-all-space
Rebase was clean, but the zebra diff didn't help, I forgot it was a bit more complex than a copy.

I reviewed the deprecated function and new class and the logic seems clearer in the new class.

I also pulled this down and manually tested. I think this is ready!

I think it's fine to RTBC since the rebase was clean and I just updated the comment referencing template_preprocess_views_view.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

10 years later, this is super close! Some minor wording fixes and let's try strict types on the new service class.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review

Ready for a new review

nicxvan’s picture

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

All feedback has been addressed!

Thanks!

amateescu’s picture

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

Added two minor points to the MR, and closed all but one of the others that have been fixed in the meantime. Feel free to self-rtbc after addressing those :)

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review

Ready for a new review

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

Latest comments addressed.

  • amateescu committed 6429cd32 on main 
    task: #2571679 Replace views_add_contextual_links() in views.module with...
amateescu’s picture

amateescu commented
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 6429cd3 and pushed to main. Thanks!

Doesn't apply cleanly to 11.x so we'll need a new MR for it.

nicxvan opened merge request !15367

nicxvan’s picture

nicxvan commented

I created a backport.

Two conflicts, one in ViewsBlockTest and one for the actual deprecation in views.module.

nicxvan’s picture

nicxvan commented
Status: Patch (to be ported) » Needs review
nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

There is an issue with the tests.

godotislate’s picture

godotislate
he/him
commented

In 11.x, view.services.yml doesn't have autowire: true in the defaults, so autowire: true needs to be added back to ViewsConfigUpdater.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Thanks! I was jumping into a meeting so I wasn't able to look at it

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Drupal\Tests\contextual\FunctionalJavascript\DuplicateContextualLinks is failing

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

That's random I think.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Yep it was random, it passes locally and running it again a few times it passed.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Reviewed & tested by the community

Compared the MRs. They look the same except the autowire per-service in services.yml which fixed 11.x

  • amateescu committed d05c9c0f on 11.x 
    task: #2571679 Replace views_add_contextual_links() in views.module with...
amateescu’s picture

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

Committed d05c9c0 and pushed to 11.x. Thanks!

I have an issue open for that specific random failure, and it would be good to get some eyes on it because it fails *a lot* lately: #3583187: [random test failure] DuplicateContextualLinksTest::testSameContextualLinks

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

amateescu closed merge request !15367

amateescu closed merge request !4577

Status: Fixed » Closed (fixed)

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