Problem/Motivation

file_progress_implementation() is a legacy function that is redundant as we only support one file upload progress method, the uploadprogress pecl extension.

Steps to reproduce

Proposed resolution

Deprecate file_progress_implementation() and replace with extension_loaded('uploadprogress').

Remaining tasks

User interface changes

API changes

file_progress_implementation() is deprecated and replaced with extension_loaded('uploadprogress').

Data model changes

Release notes snippet

Issue fork drupal-3397575

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 opened merge request !5179

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Issue summary: View changes
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

CR reads well with before/after example.

Seems all 5 instances of file_progress_implementation has been replaced.

longwave’s picture

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

I think we can improve usage of the new API in a couple of places, and is there the need for another API method, e.g. ::isAvailable() or similar that checks that some form of upload progress is available? Then the callers don't really need to know anything about the internals.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review

Re: #5 Good idea! Added and implemented in a few places.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Title: Move file_progress_implementation() to a static utility method and deprecate » Move file_progress_implementation() to a static utility class and deprecate
Issue summary: View changes
smustgrave’s picture

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

All threads do appear to be resolved. Wonder if it's too late for 10.2 though?

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Updated deprecation message to 10.3.0.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I feel like I've got to ask a couple of questions...
1. Is the static cache around extension_loaded() worth it. As far as I can see the answer is no. Locally extension_loaded('uploadprogress'); is as cheap as $a = 1 + 1;.
2. Can't we just deprecate the function as replace with extension_loaded('uploadprogress'); - we only support one implementation - as far as I know there is no plan to support another.

kim.pepper opened merge request !5757

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Title: Move file_progress_implementation() to a static utility class and deprecate » Deprecate file_progress_implementation() and replace with extension_loaded('uploadprogress')
Status: Needs work » Needs review

Makes total sense.

kim.pepper closed merge request !5179

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

I'm preeetty sure the Drupal.Tests.Core.DependencyInjection.YamlFileLoaderTest fail is not related to this issue.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Had to rebase to fix the test fail. Updated the issue summary, so I think we are good for reviews now.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Appears to have open threads.

Have not re-reviewed yet.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

There is a second upload progress implementation that #1561866: Add support for built-in PHP session upload progress is trying to add support for, although it seems tricky and may never be completed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#1561866: Add support for built-in PHP session upload progress
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Personally, I would rather not implement a new API just in case another implementation could be added in the future. I have made the mistake of over-abstracting in the past.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

+1 to #19 - especially when there is #2833968: Upload progress using jQuery.form plugin instead of 3rd party PHP libraries which potentially removes all of this...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Another thought - if we implement #1561866: Add support for built-in PHP session upload progress might it replace the uploadprogress implementation?

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

PECL upload progress is still supported and works on php 8+. Although it has the same limitation of not supporting fastcgi as session upload progress. Not sure if there is a reason for supporting one or the other.

In any case, I think we can continue with deprecating file_progress_implementation() and replacing with extension_loaded('uploadprogress') as that is what core currently supports. If and when we add another implementation, we can create an new API then.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review

Addressed feedback so back to NR.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Also added a test. Not sure if we can test the extension_loaded('uploadprogress') call. I guess we'll see what the gitlab ci images have installed.

smustgrave’s picture

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

Follows up appear to have been opened and all threads resolved.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Add a couple of comments to the MR that need adddressing.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

Thanks for continuing to push on this @kim.pepper - I've hopefully explained myself a bit more on the MR.

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

sourabhjain’s picture

sourabhjain
Primary language English
commented
Status: Needs work » Needs review
smustgrave’s picture

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

Additional feedback has been addressed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 0f7d46f and pushed to 11.x. Thanks!

  • alexpott committed 0f7d46f9 on 11.x 
    Issue #3397575 by kim.pepper, sourabhjain, alexpott, longwave: Deprecate...
kim.pepper’s picture

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

Thanks! Updated the issue summary.

Status: Fixed » Closed (fixed)

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

kim.pepper closed merge request !5757