Problem/Motivation

Three file module Hook classes have private properties, whereas the convention is to use protected. Lets change them.

  • core/modules/file/src/Hook/CronHook.php
  • core/modules/file/src/Hook/FileDownloadHook.php
  • core/modules/file/src/Hook/TokenHooks.php

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 3559980-nr-bot_6gcoabzo.txt898 bytesneeds-review-queue-bot

Issue fork drupal-3559980

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.

ritarshi_chakraborty’s picture

ritarshi_chakraborty commented

Working on it.

ritarshi_chakraborty opened merge request !13958

ritarshi_chakraborty’s picture

ritarshi_chakraborty commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Priority: Normal » Minor
Status: Needs review » Needs work
Issue tags: +Novice

Think we can expand scope to the other hooks in file.

ritarshi_chakraborty’s picture

ritarshi_chakraborty commented
Status: Needs work » Needs review
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Reviewed & tested by the community

Looks good. Did a search and couldn't find any more instances of private properties.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Shouldn't these still be readonly?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Title: Make file module CronHook use protected properties instead of private » Make file module Hook class properties protected instead of private
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Shouldn't these still be readonly?

Well, the removals weren't in-scope per the issue summary, so I'd say so. The new changes look good. RTBC again.

I updated the title and summary to mention the expanded scope per #5.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3559980-nr-bot_6gcoabzo.txt898 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

Post-bot-rebellion rebase

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 1380069e181 to main and 2fcaa7349d5 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • longwave committed 2fcaa734 on 11.x 
    refactor: #3559980 Make file module Hook class properties protected...

longwave closed merge request !13958

  • longwave committed 1380069e on main 
    refactor: #3559980 Make file module Hook class properties protected...

Status: Fixed » Closed (fixed)

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