Comments

Hardik_Patel_12 created an issue. See original summary.

hardik_patel_12’s picture

StatusFileSize
new2.28 KB

Kindly review a patch.

hardik_patel_12’s picture

Assigned: hardik_patel_12 » Unassigned
Status: Needs work » Needs review
Rangaswini’s picture

Assigned: Unassigned » Rangaswini
Rangaswini’s picture

Assigned: Rangaswini » Unassigned
Status: Needs review » Reviewed & tested by the community

Thank you @Hardik_Patel_12
Ready to update form works fine after applying a patch.

alexpott’s picture

Version: 9.0.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Needs work

When we add arguments to a constructor like this we make them optional and do a deprecation if called without. See for example the code in \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::__construct() on Drupal 8.9.x

Also new deprecations (for Drupal 10) at the moment will done once 9.1.x opens as the higher priority work is to remove deprecations from Drupal 9.

alexpott’s picture

There are also two calls to \Drupal::service('file_system') in UpdateReady.

alexpott’s picture

Title: \Drupal calls should be avoided in classes, use dependency injection instead in UpdateReady.php » \Drupal calls should be avoided in classes, use dependency injection instead in core/modules/update/src/Form classes

Re scoping... to account for #3107001: use dependency injection in UpdateManagerInstall.php and #3106993: use dependency injection in UpdateManagerUpdate.php

This issue should update all these classes together.

hardik_patel_12’s picture

StatusFileSize
new10.23 KB

Updating all these classes together and injecting using DI, Kindly review a new patch.

hardik_patel_12’s picture

Status: Needs work » Needs review
alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record
Parent issue: » #2729597: [meta] Replace \Drupal with injected services where appropriate in core

@Hardik_Patel_12 we need to do the deprecate dance as in other constructors - see #6 and because we're add new arguments to a constructor we need a change record too - the change record needs to be linked in the deprecation message too.

Also as @longwave requested in another issue I think we need to ensure that the scope here is inline with discussions on #2729597: [meta] Replace \Drupal with injected services where appropriate in core

kishor_kolekar’s picture

Assigned: Unassigned » kishor_kolekar
kishor_kolekar’s picture

Status: Needs work » Needs review
StatusFileSize
new11.97 KB

we have updated the patch with trigger message for deprecation,kindly provide us with the appropriate issue link which is currently the issue link.
@alexpott as seen in the below code example.

if (!$file_system) {
 @trigger_error('Calling UpdateManagerInstall::__construct() with the default NULL value added for $file_system argument is deprecated in drupal:9.0.0 and will be removed in drupal:10.0.0. See https://www.drupal.org/project/drupal/issues/3107003', E_USER_DEPRECATED);
    $file_system = \Drupal::service('file_system');
    }
xjm’s picture

Status: Needs review » Closed (duplicate)

Thanks for your contributions here.

I am closing this as a duplicate of #2729597: [meta] Replace \Drupal with injected services where appropriate in core. You can use the work in the patches from this and similar issues as starting points for new issues scoped by concept.