Unused variable url_options in file_token

Created on 23 June 2023, 12 months ago
Updated 13 July 2023, 11 months ago

Problem/Motivation

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

$url_options is not used, but probably should be? possible bug?

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

๐Ÿ“Œ Task
Status

Needs work

Version

11.0 ๐Ÿ”ฅ

Component
File systemย  โ†’

Last updated about 4 hours ago

Created by

๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

Live updates comments and jobs are added and updated live.
  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

Sign in to follow issues

Comments & Activities

  • Issue created by @larowlan
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia kim.pepper ๐Ÿ„โ€โ™‚๏ธ๐Ÿ‡ฆ๐Ÿ‡บSydney, Australia
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom andrew.farquharson

    andrew.farquharson โ†’ made their first commit to this issueโ€™s fork.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom andrew.farquharson

    Hi kim.pepper and larowlan,
    I have searched core codebase recursively for the line,

    $url_options = ['absolute' => TRUE];

    The line is redundant and the variable is not used in the file module. So I believe it can be safely removed.

  • last update 12 months ago
    29,551 pass
  • @andrewfarquharson opened merge request.
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom Hebl

    LGTM, but unsure whether the variable should be used.

    Changing to needs review.

  • Status changed to Needs work 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Left a comment.

  • last update 12 months ago
    29,551 pass
  • @hebl opened merge request.
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom Hebl

    Hey @smustgrave,

    Completely missed that other line, apologies! I've created a new MR which removes both lines. Thanks!

  • Status changed to RTBC 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    This is a small change but just FYI if there's already an MR should update that one vs opening a new one.

  • Status changed to Needs work 12 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    We should look through git history to find out when this line was introduced, and determine if it was later missed in a refactoring (and should be removed) or if it's a bug (and should be kept and used somewhere).

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom andrew.farquharson

    @smustgrave and @longwave I recommend a recursive search through the core folder. Search on '$url_options['language']' and you get 7 occurrences in 7 different files including file.module. Search on '$url_options = ['absolute' => TRUE];' and you get exactly 7 occurrences in the same 7 files. Both these lines are entirely redundant I believe and should be deleted from all 7 of the files (14 lines in total).

    Perhaps create related tickets for the other core modules in which the lines appear.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Shifali Baghel Gurgaon

    Hi,

    To remove unneeded variables from the file.module, I attempted this patch.

  • last update 12 months ago
    29,559 pass
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Thank you for the interest but the patch is just a copy for the MR so removing credit for duplicate work.

    Believe what longwave was asking was to review the git history for this snippet of code

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom andrew.farquharson

    I have looked through the gitlab history of the file.module file.

    The 2 lines of code that are now the subject of this issue seem to have first appeared in
    commit file.module

    The commit with SHA 988abc27f8b2f1a05e42932a39954dec4f18c09a was by Nathaniel Catchpole on 21 July 2013. In the description he refers to issue 2045189 โ†’

    So it seems that the source of the 2 $url_options lines is patch by jlindsey15 โ†’

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia sidharth_soman Bangalore

    Just to make things easier, here are the files that have the mention of the '$url_options['language']' line:

    core/lib/Drupal/Core/Utility/token.api.php
    core/modules/comment/comment.tokens.inc
    core/modules/file/filemodule.php
    core/modules/node/nodetokens.inc
    core/modules/system/system.tokens.inc
    core/modules/user/user.tokens.inc
    core/modules/views/views.tokens.inc

    I did some digging around for token.api.php and it seems like the file was created to shift the token related hooks from system.api.php. This was done on the 17th of April 2015 by Jess via issue #2470976 and commit SHA fd66ee2a9eb25ad954d5f417103127b8af280a4a

    I looked to see when the code was added in the original system.api.php and found out that the earliest addition of this code was on the 8th of January 2010 by Dries Buytaert via issue #673462 and commit SHA 4b23d00e602a1be6b7d84549bbac2888a2453ee6. As far as I know, this is where the code for hook_tokens() was added.

Production build 0.69.0 2024