Split off from #2873881: [meta] Add Change Record to @deprecated for common.inc because these types of issues need to scoped by change record not file.

To determine how to complete this issue see #2873705: Add change records to all @deprecated code

Related change record: https://www.drupal.org/node/2448603

CommentFileSizeAuthor
#6 drupal-2920003-6.patch749 bytesmairi
#4 2920003-4.patch438 bytesshashikant_chauhan

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Issue summary: View changes
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Related issues: +#2873881: [meta] Add Change Record to @deprecated for common.inc
shashikant_chauhan’s picture

shashikant_chauhan commented
Status: Active » Needs review
StatusFileSize
new 2920003-4.patch438 bytes

Adding patch.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Needs work

@shashikant_chauhan thanks for the patch
The current patch phpcs standards error "Tags must be grouped together in a doc comment."

This is because there is an existing @see \Drupal\Core\EventSubscriber\RedirectResponseSubscriber::checkRedirectUrl()

The existing @see line should be moved right before the newly added line.

Other than that it looks good!

I have checked the CR and related issues and commits. I confirmed that drupal_get_destination is the only code that is deprecated in the commit.

mairi’s picture

mairi commented
StatusFileSize
new drupal-2920003-6.patch749 bytes

Updating patch to group @see tags together.

mairi’s picture

mairi commented
Status: Needs work » Needs review
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Reviewed & tested by the community

@mairi thanks looks good!

I confirmed that patch was good except for the grouping the of @see tags. The url for the CR is the same so RTBC! 🎉

xjm credited dinarcon.

xjm credited kwhite.

xjm’s picture

xjm
she/her
Primary language English
commented

Updating credit.

  • xjm committed 18205e8 on 8.4.x 
    Issue #2920003 by mairi, shashikant_chauhan, tedbow, dinarcon, kwhite:...

  • xjm committed 3973f7f on 8.4.x 
    Revert "Issue #2920003 by mairi, shashikant_chauhan, tedbow, dinarcon,...

  • xjm committed 9830ef9 on 8.5.x 
    Issue #2920003 by mairi, shashikant_chauhan, tedbow, dinarcon, kwhite:...

  • xjm committed 84da110 on 8.4.x 
    Issue #2920003 by mairi, shashikant_chauhan, tedbow, dinarcon, kwhite:...
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks for working on these deprecations @mairi and @shashikant_chauhan!

I checked and confirmed that this is only API change from the original issue, and that the correct change record is linked in the patch.

Note that I accidentally committed this to 8.4.x only at first, so that's what the revert and recommit is about.

Status: Fixed » Closed (fixed)

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

kay_v’s picture

kay_v commented
Parent issue: #2873705: Add change records to all @deprecated code »

Removing parent issue per conversation with @xjm at Drupalcon Nashville Mentored Sprint prep. Her recommendation to do so was based on a few points that made sense to all of us in the discussion, namely:
- so many child issues makes this parent unwieldy
- search filters will allow people needing to refind closed children