Followup to #2158871: [Policy, no patch] Clearly denote when @deprecated code is slated to be removed

All functions labelled @deprecated should include details about when they were deprecated, when they will be removed and, where possible, what they should be replaced with.

For example:

 * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
 *   Use \Drupal\content_translation\Controller\ContentTranslationController::edit().

Some functions should not be changed at this stage. These are:

See also https://drupal.org/coding-standards/docs#deprecated

CommentFileSizeAuthor
#25 2187735-25-deprecated-docblocks.patch74.02 KBianthomas_uk
#20 interdiff-16-20.txt38.3 KBianthomas_uk
#20 2187735-20-deprecated-docblocks.patch74.72 KBianthomas_uk
#16 interdiff-9-16.txt13.34 KBianthomas_uk
#16 2187735-16-deprecated-docblocks.patch114.26 KBianthomas_uk
#9 2187735-9-deprecated-docblocks.patch115.72 KBianthomas_uk
#8 2187735-8-deprecated-docblocks.patch116.25 KBSutharsan
#3 ajax.patch6.46 KBandypost
#2 2187735-2-deprecated-docblocks.patch117.29 KBianthomas_uk
#1 2187735-1-deprecated-docblocks.patch73.44 KBianthomas_uk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
FileSize
2187735-1-deprecated-docblocks.patch73.44 KB

Here's my first pass of the modules folder.

I've tried to keep the format consistent:
- All state '@deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
- Class names are fully qualified.
- The word 'Use' is normally on the second line for easier reading, except when there are multiple possible replacements, where each replacement gets its own line.
- I've removed the word 'instead' where it appears and added full stops to the end of sentences.
- I've removed @see when it just duplicates the information in the @deprecated.
- I've added @see when using \Drupal if an intermediate service is used (as ideally the service should be used directly).
- I've removed suggestions to inject services rather than using \Drupal.

These are all open for discussion, particularly the last point. It was clumsy the way it was being used, but the phrasing I've used makes no reference to dependency injection. Maybe we should link to a doc page about doing this properly?

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Assigned: ianthomas_uk » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
2187735-2-deprecated-docblocks.patch117.29 KB

This patch updates everything currently marked @deprecated, except where mentioned in the issue summary.

It's a big patch, but is very repetitive, so partial reviews are welcome.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
ajax.patch6.46 KB

Adds code for regression from #1959574-86: Remove the deprecated Drupal 7 Ajax API
EDIT should be commited

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

@andypost this issue is about changing the docblock of functions already marked as @deprecated, I think you may have confused it with another issue.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

@andypost
What about fixing it first in the other issue?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

@dawehner, yep, is new issue needed to fix regression?

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

Any thoughts on the patch? It's extremely repetitive, so even a review of one function is useful, as the same thoughts are likely to apply to 90% of the patch.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented
FileSize
2187735-8-deprecated-docblocks.patch116.25 KB

Rerolled the #2 patch.
The following functions got removed from core in the meantime:

  • entity_get_form()
  • ajax_test_dialog()
  • ajax_test_dialog_form()
  • ajax_test_dialog_form_submit()
  • ajax_test_dialog_form_callback_modal()
  • ajax_test_dialog_form_callback_nonmodal()
  • _ajax_test_dialog()

Review of #2 patch: 

  1. +++ b/core/includes/config.inc
@@ -54,7 +54,7 @@ function config_get_storage_names_with_prefix($prefix = '') {
- * @deprecated as of Drupal 8.0. Use \Drupal::config() instead.
+ * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0. Use \Drupal::config().

    Comment longer than 80 chars. 

  2. +++ b/core/includes/entity.inc
@@ -460,8 +463,8 @@ function entity_access_controller($entity_type) {
- * @deprecated Use \Drupal::entityManager()->getForm() or _entity_form from a
- *   routing.yml file instead of a page callback.
+ * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
+ *   Use \Drupal::entityManager()->getForm() or _entity_form from a routing.yml file instead of a page callback.

    Again 

  3. +++ b/core/lib/Drupal.php
@@ -111,8 +111,8 @@ public static function setContainer(ContainerInterface $container) {
-   * @deprecated This method is only useful for the testing environment. It
-   * should not be used otherwise.
+   * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
+   *   This method is only useful for the testing environment. It should not be used otherwise.

    Again 

  4. +++ b/core/modules/system/system.module
@@ -2696,7 +2696,8 @@ function system_rebuild_module_data() {
- * @deprecated as of Drupal 8.0. Use \Drupal::service('theme_handler')->rebuildThemeData().
+ * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
+ *   Use \Drupal::service('theme_handler')->rebuildThemeData().

    Some 'Use ...' lines end with a period, some don't. Please make consistent.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
FileSize
2187735-9-deprecated-docblocks.patch115.72 KB

Here's a reroll that removes two more functions that were removed from forum_test.module.

RE #8
1) I'm not sure which line you thought was over 80 chars. There was one that the patch didn't touch, so I've fixed that in passing.
2) I can't see any lines over 80 chars.
3) Fixed.
4) Fixed.

Status: Needs review » Needs work

The last submitted patch, 9: 2187735-9-deprecated-docblocks.patch, failed testing.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Status: Needs work » Needs review

9: 2187735-9-deprecated-docblocks.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2187735-9-deprecated-docblocks.patch, failed testing.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented
Status: Needs work » Needs review

9: 2187735-9-deprecated-docblocks.patch queued for re-testing.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented 
+++ b/core/includes/config.inc
@@ -22,13 +22,13 @@ function config_get_storage_names_with_prefix($prefix = '') {
- * @deprecated Deprecated since Drupal 8.x-dev, to be removed in Drupal 8.0.
- *   Use \Drupal::config() instead.
+ * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0. ¶
+ *  Use \Drupal::config().

Just a minor one: a trailing white space.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Why some of the functions have @see same as @deprecated and some haven't? Please make it consistent.

  1. +++ b/core/modules/field/field.api.php
@@ -318,7 +318,8 @@ function hook_field_formatter_info_alter(array &$info) {
+ *   Use the entity system instead, see https://drupal.org/developing/api/entity.

@@ -346,7 +347,8 @@ function hook_field_attach_form(\Drupal\Core\Entity\EntityInterface $entity, &$f
+ *   Use the entity system instead, see https://drupal.org/developing/api/entity.

@@ -377,7 +379,8 @@ function hook_field_attach_extract_form_values(\Drupal\Core\Entity\EntityInterfa
+ *   Use the entity system instead, see https://drupal.org/developing/api/entity.

    more then 80 chars. 

  2. +++ b/core/modules/field/field.deprecated.inc
@@ -273,7 +273,8 @@ function field_info_instance($entity_type, $field_name, $bundle_name) {
+ *   Use the entity system instead, see https://drupal.org/developing/api/entity.

@@ -328,7 +329,8 @@ function field_attach_form(EntityInterface $entity, &$form, &$form_state, $langc
+ *   Use the entity system instead, see https://drupal.org/developing/api/entity.

@@ -372,7 +374,8 @@ function field_attach_form_validate(ContentEntityInterface $entity, $form, &$for
+ *   Use the entity system instead, see https://drupal.org/developing/api/entity.

@@ -412,7 +415,8 @@ function field_attach_extract_form_values(EntityInterface $entity, $form, &$form
+ *   Use the entity system instead, see https://drupal.org/developing/api/entity.

@@ -467,7 +471,8 @@ function field_attach_prepare_view($entity_type, array $entities, array $display
+ *   Use the entity system instead, see https://drupal.org/developing/api/entity.

    More then 80 chars.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
FileSize
2187735-16-deprecated-docblocks.patch114.26 KB
interdiff-9-16.txt13.34 KB

- Reroll following #2167267: Remove deprecated field_attach_*_view() (interdiff was generated pre-reroll).
- Addresses #14.
- I've removed the full stop from those 81 character lines, as I thought that looked better than wrapping the whole URL.
- Generally relevant classes are mentioned in @return or @deprecated and I don't think it's necessary to duplicated that in an @see. Where a method is a wrapper around a \Drupal method the intermediate handler isn't usually mentioned in the docblock, so an @see will normally be used. I've fixed this in several places, including suggesting \Drupal as a replacement in more places, and moving the old suggested replacement to @see.
- I've removed the change to getContainer(), as I think that's actually private rather than deprecated. See #2160655-24: Improve error handling of \Drupal class
- I'm not convinced the suggested replacements in the various tests in the system module are correct, but I'm just moving around the existing suggestions and I think correcting them is outside the scope of this issue.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/includes/common.inc
@@ -984,7 +991,10 @@ function format_size($size, $langcode = NULL) {
+ * @see \Drupal\Core\Datetime\Date::formatInterval()

    We don't add the @see everywhere, but I think this is totally fine. 

  2. +++ b/core/includes/theme.inc
@@ -72,7 +72,10 @@
+ * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
+ *   Use \Drupal::service('access_check.theme')->checkAccess().
+ *
+ * @see \Drupal\Core\Theme\ThemeAccessCheck::checkAccess().

    Note: sometimes we use @deprected and then @see and the other way round. I would think that @see should be above, what do you think? 

  3. +++ b/core/modules/system/tests/modules/form_test/form_test.module
@@ -2112,7 +2150,8 @@ function form_test_group_fieldset() {
+ *   Use \Drupal\form_test\testGroupVerticalTabs().
...
 function form_test_group_vertical_tabs() {

    Note: All those should be actually \Drupal\form_test\Form\FormTestForm::testGroupVerticalTabs() instead, but this is some bug already in HEAD.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

We don't add the @see everywhere, but I think this is totally fine.

I've tried to include @see everywhere that it is different from @return or @deprecated. In the example given, @deprecated lists \Drupal::service('date')->formatInterval() and the @see is for \Drupal\Core\Datetime\Date::formatInterval().

sometimes we use @deprected and then @see and the other way round. I would think that @see should be above, what do you think?

I'd probably put @see at the very end of all the comments. But this is already inconsistent across core and I think would add a lot of work to fix for not much benefit.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

It is odd that we use @deprecated for tests anyway.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
FileSize
2187735-20-deprecated-docblocks.patch74.72 KB
interdiff-16-20.txt38.3 KB

That's a good point. I'll split the tests out into a different issue, as we might want to handle those in a different way and they shouldn't cause updates to documentation of APIs that people are actually using to be delayed.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

Is there any reason to mark private functions as deprecated? Those are not supported anyway, so I don't see a need to bother with them.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

If other people just want the @deprecated tags to be removed from private functions then I'd be fine with that, but I don't think they are doing any harm.

Strictly speaking we probably don't need them, but they can still be useful to core devs and are something we can check for as we approach 8.0 (i.e. are there any @deprecated functions left? If so, why?). It's also an extra hint that people shouldn't be using these functions.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Let's be pragmatic and get it in.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

2187735-20-deprecated-docblocks.patch no longer applies.

error: patch failed: core/includes/entity.inc:23
error: core/includes/entity.inc: patch does not apply
ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Status: Needs work » Reviewed & tested by the community
FileSize
2187735-25-deprecated-docblocks.patch74.02 KB

Reroll - just needed to ignore the failed hunk because entity_get_info() has been removed for #1981858: Rename hook_entity_info/alter() to hook_entity_type_build/alter()

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Assigned: Unassigned » ianthomas_uk
Status: Reviewed & tested by the community » Needs work

Needs another reroll - leave it with me

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Assigned: ianthomas_uk » Unassigned
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Actually, no reroll needed (it was just git getting confused)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed eb53ecc and pushed to 8.x. Thanks!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Sadly the fix from andypost got lost again: #2203239: Remove ajax_render and co.

Status: Fixed » Closed (fixed)

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