CommentFileSizeAuthor
#16 drupal-deprecated_link_change_record-2873746-16.patch964 bytesmaryedith
#9 drupal-deprecated_link_change_record-2873746-9.patch481 bytesjohnshortess
#6 drupal-deprecated_link_change_record-2873746-4.patch481 bytesjohnshortess
#5 drupal-deprecated_link_change_record-2873746-3.patch481 bytesjohnshortess
#2 drupal-deprecated_link_change_record-2873746-2.patch477 bytesmaryedith

Comments

maryedith created an issue. See original summary.

maryedith’s picture

maryedith commented
Status: Active » Needs review
StatusFileSize
new drupal-deprecated_link_change_record-2873746-2.patch477 bytes

adding patch to link @deprecated to change record

johnshortess’s picture

johnshortess
he/him
Primary language English
Location Fairfax, Virginia, USA
commented
Status: Needs review » Reviewed & tested by the community

This looks good to me. The added comment appears to be linking to the correct change record, the format matches that in the parent issue, and there's no stray whitespace.

johnshortess’s picture

johnshortess
he/him
Primary language English
Location Fairfax, Virginia, USA
commented
Status: Reviewed & tested by the community » Needs work

Oops, missed that the format in the parent issue has a blank line before the @see.

johnshortess’s picture

johnshortess
he/him
Primary language English
Location Fairfax, Virginia, USA
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-deprecated_link_change_record-2873746-3.patch481 bytes

Here's a new patch with the added blank line.

johnshortess’s picture

johnshortess
he/him
Primary language English
Location Fairfax, Virginia, USA
commented
StatusFileSize
new drupal-deprecated_link_change_record-2873746-4.patch481 bytes

Wrong patch. Here's the right one.

The last submitted patch, 5: drupal-deprecated_link_change_record-2873746-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: drupal-deprecated_link_change_record-2873746-4.patch, failed testing.

johnshortess’s picture

johnshortess
he/him
Primary language English
Location Fairfax, Virginia, USA
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-deprecated_link_change_record-2873746-9.patch481 bytes

Re-roll.

edwdeapri’s picture

edwdeapri commented

Reviewing now - Baltimore

edwdeapri’s picture

edwdeapri commented

Successfully applied patch. Reviewed changes to code. There are further @deprecated instances within file. Line 122 and 135.

edwdeapri’s picture

edwdeapri commented
Status: Needs review » Needs work
maryedith’s picture

maryedith commented
Issue tags: +Yes, +the other 2 @deprecated lines in the code don't exactly fit under the same change record as the first, +and there is not an exactly matching change record. Shall we make a change record for menu_primary_local_tasks() and menu_secondary_local_tasks()?

Yes, -the other 2 @deprecated lines in the code don't exactly fit under the same change record as the first, -and there is not an exactly matching change record. Shall we make a change record for menu_primary_local_tasks() and menu_secondary_local_tasks()?

edwdeapri’s picture

edwdeapri commented

@maryedith - Yes. During DrupalCon Baltimore sprints, we were asked that all @deprecated lines needed an original corresponding change record. New ones would need to be created for change records that didn't exist.

quietone’s picture

quietone commented
Component: migration system » menu system
Issue tags: -Yes, -the other 2 @deprecated lines in the code don't exactly fit under the same change record as the first, -and there is not an exactly matching change record. Shall we make a change record for menu_primary_local_tasks() and menu_secondary_local_tasks()?

Hey, thanks for doing all this work on the deprecations and change records.

The changes are to menu, so moving to the menu system.

@maryedith, it looks like you meant to make a comment and not add a long issue tag in #13. Maybe you could edit your comment?

Edit: Sorry, I didn't intend to delete the issue tag.

This is the text that was in the issue tag and really looks like it should have been a comment in #13.
Yes, -the other 2 @deprecated lines in the code don't exactly fit under the same change record as the first, -and there is not an exactly matching change record. Shall we make a change record for menu_primary_local_tasks() and menu_secondary_local_tasks()?

maryedith’s picture

maryedith commented
Status: Needs work » Needs review
StatusFileSize
new drupal-deprecated_link_change_record-2873746-16.patch964 bytes

Ok, I tried my hand at making a change record:
https://www.drupal.org/node/2874695.

this change record applies to both menu_primary_local_tasks() and menu_secondary_local_tasks()

maryedith’s picture

maryedith commented
Title: Add Change record to @deprecated for menu.inc on line 110 » Add Change record to @deprecated for menu.inc

changed title of this issue to not include the line #

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Reviewed & tested by the community

Thanks for making a change record https://www.drupal.org/node/2874695

It really shouldn't be published before this issue is fixed, but no biggie. It does tell you how to work around the deprecation, but it doesn't link to the issues in question.

Updated the CR. Maintainer please remember to publish it. :-)

Added this issue to existing CR https://www.drupal.org/node/2544940

Code patch applies and adds appropriate @see, checked using git -L.

  • catch committed 4ba925c on 8.4.x 
    Issue #2873746 by johnshortess, maryedith, edwdeapri, Mile23: Add Change...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

In this case it'd be fine for the change record to be published since the deprecation already happened, it was just missing the change record.

Committed 4ba925c and pushed to 8.4.x. Thanks!

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