Problem/Motivation

Follow-up to #3493951: Split File oop hook implementations into separate classes where I noticed we have the following code in \Drupal\file\Hook\CronHook::__invoke():

    $url_options = ['absolute' => TRUE];
    if (isset($options['langcode'])) {
      $url_options['language'] = $this->languageManager->getLanguage($options['langcode']);
      $langcode = $options['langcode'];
    }
    else {
      $langcode = NULL;
    }

However, $url_options is never read anywhere, which means \Drupal\Core\Language\LanguageManagerInterface::getLanguage() is being called unneccessarily and the LanguageManagerInterface dependency can be removed.

Steps to reproduce

Proposed resolution

Remove the $url_options variable and LanguageManagerInterface dependency.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3493958

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:

About issue forks

Comments

kim.pepper created an issue. See original summary.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Issue summary: View changes

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

jquijano’s picture

jquijano commented

I will work on this

jquijano opened merge request !10553

jquijano’s picture

jquijano commented

Removed $url_options as suggested, LanguageManagerInterface was not referenced in code.

jquijano’s picture

jquijano commented
Status: Active » Needs review
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Title: Remove File token hook dead code » [PP-1] Remove File token hook dead code
Status: Needs review » Postponed

Sorry I should have postponed this on #3493951: Split File oop hook implementations into separate classes

dcam’s picture

dcam commented
Title: [PP-1] Remove File token hook dead code » Remove File token hook dead code
Status: Postponed » Needs work
Issue tags: +Novice

The related issue to split up the File hooks was committed. The changes in the existing MR will need to be applied in the new TokenHooks class. This seems like a Novice task to me.

jquijano’s picture

jquijano commented
Assigned: Unassigned » jquijano

ok, i'll work on this

jquijano’s picture

jquijano commented
Status: Needs work » Active
jquijano’s picture

jquijano commented
Assigned: jquijano » Unassigned
Status: Active » Needs review

Rebased branch to incorporate the latest updates. Made changes to the TokenHook class. Note: The CronHook file has been rewritten due to updates, rendering the original modifications unnecessary.

adwivedi008’s picture

adwivedi008 commented
Status: Needs review » Reviewed & tested by the community

Hello @jquijano,

I have checked the changes and it looks good to me
Also, there is no merge conflict as you already rebased the branch

Seems good to me, moving the issue to RTBC and let's wait for somemore feedback

quietone’s picture

quietone commented

Whenever dead code is removed we should do some digging to make sure that we aren't accidentally removing the last remnant of something that should be implemented. It is possible that this is accidentally dead code.

Therefor I used git log -L 762,767:core/modules/file/file.module to find out when $url_options was added. It was in 

commit 988abc27f8b2f1a05e42932a39954dec4f18c09a
Author: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date:   Wed Jul 24 13:00:06 2013 +0100

    Issue #2045189 by jlindsey15: Move file entity dependent code in includes/file.inc and system.module to file.module.

I then read that issue and found $url_options in the first patch there. It was removed in the next patch after feedback from berdir. It was originally used in the several of the cases following the line of code and those were re-written. I think that confirms that we are not losing anything here.

  • quietone committed 56e3f8e0 on 11.x 
    Issue #3493958 by jquijano, kim.pepper, dcam, adwivedi008: Remove File...
quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Fixed

Committed 56e3f8e and pushed to 11.x.

Thanks!

quietone closed merge request !10553

Status: Fixed » Closed (fixed)

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