adding change records for all @deprecated constants in .module files

CommentFileSizeAuthor
#13 2873768-13.patch11.71 KBharsha012
#7 2873768-7.patch12.82 KBWidgetsBurritos
#7 interdiff-5-7.txt1.47 KBWidgetsBurritos
#5 drupal-change_record_deprecated-2873768-5.patch10.29 KBdhopki12
#3 drupal-change_record_deprecated-2873768-3.patch7.8 KBdhopki12
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dhopki12 created an issue. See original summary.

dhopki12’s picture

dhopki12 CreditAttribution: dhopki12 commented
Assigned: Unassigned » dhopki12
dhopki12’s picture

dhopki12 CreditAttribution: dhopki12 commented
Status: Active » Needs review
FileSize
drupal-change_record_deprecated-2873768-3.patch7.8 KB

Adding patch

I added an "@see https://www.drupal.org/node/2831620" message to each constant in each file listed in the original change record. After adding this patch, I reviewed other *.module files in core and found 3 more with constants listed as deprecated which are not included in the original record.

  • taxonomy.module
  • image.module
  • node.module

I tracked down the following change orders and am going to submit a revised patch after updating these files with appropriate @see statements as well.

Have not found the change order referencing image.module constants

dhopki12’s picture

dhopki12 CreditAttribution: dhopki12 commented
Status: Needs review » Needs work
dhopki12’s picture

dhopki12 CreditAttribution: dhopki12 commented
Status: Needs work » Needs review
FileSize
drupal-change_record_deprecated-2873768-5.patch10.29 KB

Adding new patch addressing taxonomy.module and node.module file constants. Still have not found the change order referencing image.module constants.

WidgetsBurritos’s picture

WidgetsBurritos CreditAttribution: WidgetsBurritos at Rackspace commented
Status: Needs review » Needs work

@dhopki12,

From the looks of it, this is the CR for image.module:
[#1820974]

IMAGE_STORAGE_* constants have been deprecated and will be removed before 9.0.x

I don't have a chance right this second to update the patch, but I can either jump on it this weekend sometime, or if you wanna update your patch, I think the rest of this looks pretty good.

WidgetsBurritos’s picture

WidgetsBurritos CreditAttribution: WidgetsBurritos at Rackspace commented
Assigned: dhopki12 » Unassigned
Status: Needs work » Needs review
FileSize
interdiff-5-7.txt1.47 KB
2873768-7.patch12.82 KB

This patch adds the CR for the image module. I believe this to be the last module file needed in this patch.

That said, the image CR (https://www.drupal.org/node/1820974) merely makes mention of the deprecation of IMAGE_STORAGE_* modules and doesn't necessarily provide a detailed explanation of which modules were deprecated as was done for other modules, but it did seem to get approved as part of the removal itself: #2637630: Deprecate unused constants in image.module.

Either way, it would be easy enough to modify the CR if anyone sees the need to do so.

pobster’s picture

pobster CreditAttribution: pobster at ArcadeGeek LTD for Rackspace commented

LGTM +1

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Verified that all the @deprecated constants in *.module files are covered.

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

This is failing DrupalCs checks - tl;dr there shouldn't be an empty line between the @deprecated and the @see:

FILE: /Users/catch/Sites/8.x/core/modules/node/node.module
----------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
----------------------------------------------------------------------
 40 | ERROR | [x] Additional blank lines found at end of doc comment
 51 | ERROR | [x] Additional blank lines found at end of doc comment
 62 | ERROR | [x] Additional blank lines found at end of doc comment
 73 | ERROR | [x] Additional blank lines found at end of doc comment
 84 | ERROR | [x] Additional blank lines found at end of doc comment
 95 | ERROR | [x] Additional blank lines found at end of doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

(and etc.)

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

I think you mean there shouldn't be an extra line *after* @see.

harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
Status: Needs work » Needs review
FileSize
2873768-13.patch11.71 KB

re-roll the patch against 8.5x and fixed the spacing issue

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

The patch in #13 applies and doesn't show any errors in phpcs.

Do we still need to modify the CR as per #7?

If we do, we'd want to be sure and link it back to this issue.

angel.h’s picture

angel.h
Primary language English
Location Plovdiv
CreditAttribution: angel.h at FFW commented
Assigned: Unassigned » angel.h
Issue tags: -Baltimore2017 +Vienna2017
angel.h’s picture

angel.h
Primary language English
Location Plovdiv
CreditAttribution: angel.h at FFW commented
Assigned: angel.h » Unassigned
Status: Needs review » Reviewed & tested by the community

All fine.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

I agree with #7, added this sentence to the image CR: https://www.drupal.org/node/1820974/revisions/view/9222920/10671248

There are no replacements given the concepts are different.

Also checked that all the links point to the correct change records for each constant linked.

  • Gábor Hojtsy committed 93b36cb on 8.5.x 
    Issue #2873768 by dhopki12, WidgetsBurritos, harsha012, Mile23, catch:...

  • Gábor Hojtsy committed 471fd77 on 8.4.x 
    Issue #2873768 by dhopki12, WidgetsBurritos, harsha012, Mile23, catch:...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks all, committed to 8.5.x and cherry-picked to 8.4.x.

Status: Fixed » Closed (fixed)

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

kay_v’s picture

kay_v CreditAttribution: kay_v as a volunteer 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