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

Comments

ianthomas_uk’s picture

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

Assigned: ianthomas_uk » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
117.29 KB
PASSED: [[SimpleTest]]: [MySQL] 63,599 pass(es). View

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

FileSize
6.46 KB
PASSED: [[SimpleTest]]: [MySQL] 63,627 pass(es). View

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

ianthomas_uk’s picture

@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

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

andypost’s picture

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

ianthomas_uk’s picture

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

FileSize
116.25 KB
PASSED: [[SimpleTest]]: [MySQL] 64,350 pass(es). View

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

FileSize
115.72 KB
PASSED: [[SimpleTest]]: [MySQL] 64,259 pass(es). View

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

Status: Needs work » Needs review

Status: Needs review » Needs work

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

Sutharsan’s picture

Status: Needs work » Needs review
Sutharsan’s picture

+++ 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

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

FileSize
114.26 KB
PASSED: [[SimpleTest]]: [MySQL] 64,409 pass(es). View
13.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

  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

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

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

ianthomas_uk’s picture

FileSize
74.72 KB
PASSED: [[SimpleTest]]: [MySQL] 64,424 pass(es). View
38.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

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

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

Status: Needs review » Reviewed & tested by the community

Let's be pragmatic and get it in.

alexpott’s picture

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

Status: Needs work » Reviewed & tested by the community
FileSize
74.02 KB
PASSED: [[SimpleTest]]: [MySQL] 64,591 pass(es). View

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

Assigned: Unassigned » ianthomas_uk
Status: Reviewed & tested by the community » Needs work

Needs another reroll - leave it with me

ianthomas_uk’s picture

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

Status: Reviewed & tested by the community » Fixed

Committed eb53ecc and pushed to 8.x. Thanks!

dawehner’s picture

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.