Problem

Class ModuleUpdater defines a method getSchemaUpdates() used nowhere -- we may want to remove it.

That method itself calls a function _update_get_project_type() defined nowhere.

Solution

Drop the method (see patch attached).

Credits

My colleague @karolinam reported this first.

Issue fork drupal-3050578

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

fengtan created an issue. See original summary.

fengtan’s picture

Status: Active » Needs review
StatusFileSize
new1.12 KB
poker10’s picture

Version: 7.x-dev » 10.1.x-dev

This function is still in Drupal 10.1.x-dev, see: https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/lib/Drupal/Core/Updater/Module.php#L84

It seems like it has no direct usages in D10 as well. We need to probably clarify the purpose of this function to decide.

smustgrave’s picture

Status: Needs review » Needs work

Searching the repo I can't find any reference.

Searched http://grep.xnddx.ru/search?text=getSchemaUpdates&filename= and also didn't find any reference. Probably safe to remove as dead code.

nitin_lama’s picture

Assigned: Unassigned » nitin_lama
nitin_lama’s picture

Status: Needs work » Needs review
StatusFileSize
new1.16 KB

If we need to remove the unused code, here's a patch to remove it. I couldn't find any reference in the core as well.

nitin_lama’s picture

Assigned: nitin_lama » Unassigned
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Lets see what the committer says.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Before removing something that's unused, we should check whether it's used in contrib (in this case no), but also why it's unused.

I ran git log -S getSchemaUpdates and the last mention was

commit 368cf30de0237484dd8eef86069b6649faf66347
Author: webchick <webchick@24967.no-reply.drupal.org>
Date:   Sat Mar 10 16:23:05 2012 -0800

    Issue #1471376 by amateescu: Convert updater.inc to PSR-0.


I went back to the first mention:

git show 6abcc47e25e936aea8 | grep getSchemaUpdates

And it shows the method being added but no usages.

As far as I can tell this was unused when it was added in 2009 and we've just been occasionally moving it around since.

However, it's a public method on a core class so I think we should deprecate it first.

elber made their first commit to this issue’s fork.

elber’s picture

I will work on it.

elber’s picture

Status: Needs work » Needs review
StatusFileSize
new671 bytes

Hi I deprecated the method and also created a change record, please

https://www.drupal.org/node/3359445

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

If we are going to deprecate it think we should also add trigger_error for it.

elber’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Should follow how other methods are deprecated

@trigger_error(__METHOD__ . '() is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. There is no replacement. See https://www.drupal.org/node/3337942', E_USER_DEPRECATED);

andypost’s picture

unpublished CR, it needs deprecation test additionally to #16 https://www.drupal.org/about/core/policies/core-change-policies/drupal-d...

catch’s picture

We don't need a deprecation test unless there's an actual bc layer with logic in it or similar, when the deprecation is 'just' the phpdoc and trigger_error() we can skip testing for those.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone made their first commit to this issue’s fork.

quietone’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Trigger_error looks good, right version.

CR reads well and is with the right branches.

Think this is good to go.

catch’s picture

Status: Reviewed & tested by the community » Needs work

I think this should just be deprecated with no replacement, it's not replaced by hook_update_N(), it's about finding out whether a module has updates that need to be run. No-one is likely to use this since it's very internal to core (except maybe drush but they won't be using this either).

Or we could point people to \Drupal::service('update.update_hook_registry')->getAvailableUpdates

quietone’s picture

Status: Needs work » Needs review

I went with giving people an option, to use \Drupal::service('update.update_hook_registry')->getAvailableUpdates. I also updated the CR.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Thanks that looks good, back to RTBC.

andypost’s picture

Using MR, RTBC++

longwave’s picture

Crediting @karolinam as requested in the IS.

longwave’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8e42cef and pushed to 11.x. Thanks!

Also published the change record.

  • longwave committed 8e42cef1 on 11.x
    Issue #3050578 by elber, quietone, nitin_lama, fengtan, smustgrave,...

Status: Fixed » Closed (fixed)

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