Follow-up to #2587755: AJAX error when using progress bar on file field widget

Problem/Motivation

The file module's FileWidgetAjaxController class is a bit ugly/messy.

Solution

Refactor. Mostly drafted by chx, discussed on IRC (so he should get a credit on commit assuming this gets commited). There was also some discussion on eligibility for commiting into 8; no conclusion was reached but brought up was:
- until/before #2587755: AJAX error when using progress bar on file field widget it didn't work (returned a 500 HTTP response)
- therefore there shouldn't be any disruption
- controllers are not API surface
- Any contrib usage would have had to change the path so probably would have just created a new route and as long as it passed the key to this class it would work with this refactoring.

CommentFileSizeAuthor
#36 interdiff_34-36.txt730 bytessahil.goyal
#36 2615678-36.patch3.13 KBsahil.goyal
#34 reroll_diff_18-34.txt2.86 KBravi.shankar
#34 2615678-34.patch3.09 KBravi.shankar
#33 FixWidgetAjaxControl.png2.62 MBrudrakumar188
#18 FileWidgetAjaxController-2615678-18.patch3.52 KBChuChuNaKu
#9 FileWidgetAjaxController-2615678-9.patch3.52 KBmiteshmap
#2 FileWidgetAjaxController-2615678-2.patch3.52 KBnickdickinsonwilde

Comments

NickWilde created an issue. See original summary.

nickdickinsonwilde’s picture

nickdickinsonwilde
He/Him/His
Primary language English
Location Victoria, BC (T'So-uke lands)
CreditAttribution: nickdickinsonwilde commented
Title: AJAX error when using progress bar on file field widget » Improve FileWidgetAjaxController
Component: ajax system » file.module
Category: Bug report » Task
Priority: Major » Normal
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new FileWidgetAjaxController-2615678-2.patch3.52 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

While this seems to be a fair refactoring I don't even remotely get why this should be done before the release ... this could totally happy land afterwards

chx’s picture

chx CreditAttribution: chx commented
Issue tags: -rc target triage

I guess. I can't RTBC because I wrote it :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php
@@ -15,36 +15,74 @@
+   *  An array with upload status information retrieved from the file
+   *  progess implementation.
...
+   *  The array key to use to get the current upload progess (in bytes)
+   *  from $status.
...
+   *   The array key to use to get the total bytes to upload from $status.

Nitpick: These bits needs one more space of indentation, but this can be also fixed on commit.

nickdickinsonwilde’s picture

nickdickinsonwilde
He/Him/His
Primary language English
Location Victoria, BC (T'So-uke lands)
CreditAttribution: nickdickinsonwilde commented

@dawehner: those lines would be my fault (rather than chx's)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Reviewed & tested by the community » Needs work

Looks like a good cleanup; just a couple coding standards nitpicks for now:

  1. +++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php
@@ -15,36 +15,74 @@
-   * Returns the progress status for a file upload process.
+   * Determines the file progress implementation available and passes
+   * the $key on to the implementation wrapper.

    This needs to be a single line of fewer than 80 characters. 

  2. +++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php
@@ -15,36 +15,74 @@
+   * @see file_progress_implementation().

    No period at the end of an @see.

Do we have test coverage for this? Does #2587755: AJAX error when using progress bar on file field widget add some? Edit: Not yet at least. :)

swentel’s picture

swentel CreditAttribution: swentel commented
Issue tags: +Novice

Additional small things to fix. The other issue has tests now, so no more needed here then.

  1. +++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php
@@ -15,36 +15,74 @@
+   *  progess implementation.

    progess -> progress 

  2. +++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php
@@ -15,36 +15,74 @@
+   *  The array key to use to get the current upload progess (in bytes)

    progess -> progress

miteshmap’s picture

miteshmap
Primary language Gujarati
CreditAttribution: miteshmap as a volunteer commented
Status: Needs work » Needs review
StatusFileSize
new FileWidgetAjaxController-2615678-9.patch3.52 KB

All changes are considered addressed by: @dawehner, @xjm and @swentel

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC then.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: FileWidgetAjaxController-2615678-9.patch, failed testing.

swentel’s picture

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

Bot hickup

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: FileWidgetAjaxController-2615678-9.patch, failed testing.

mikemiles86’s picture

mikemiles86
he/him
CreditAttribution: mikemiles86 as a volunteer commented
Issue tags: +SprintWeekend2106, +SprintWeekendBOS
chx’s picture

chx CreditAttribution: chx commented
Issue tags: -SprintWeekend2106 +SprintWeekend2016
lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Needs work » Needs review

retest.

ChuChuNaKu’s picture

ChuChuNaKu CreditAttribution: ChuChuNaKu as a volunteer commented
Assigned: Unassigned » ChuChuNaKu
ChuChuNaKu’s picture

ChuChuNaKu CreditAttribution: ChuChuNaKu as a volunteer commented
Assigned: ChuChuNaKu » Unassigned
StatusFileSize
new FileWidgetAjaxController-2615678-18.patch3.52 KB

re-rolled patch.

Status: Needs review » Needs work

The last submitted patch, 18: FileWidgetAjaxController-2615678-18.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented

Seems to have failed due to the uploadprogress library not being installed. I don't have the library installed locally either.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

rudrakumar188’s picture

rudrakumar188 CreditAttribution: rudrakumar188 at Material for Drupal India Association commented
Status: Needs review » Needs work
StatusFileSize
new FixWidgetAjaxControl.png2.62 MB

The patch #18 cannot be applied getting lots of error. Adding screenshot of errors.

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Needs work » Needs review
StatusFileSize
new 2615678-34.patch3.09 KB
new reroll_diff_18-34.txt2.86 KB

Added reroll of patch #18 on Drupal 9.4.x.

Status: Needs review » Needs work

The last submitted patch, 34: 2615678-34.patch, failed testing. View results

sahil.goyal’s picture

sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Material for Drupal India Association commented
StatusFileSize
new 2615678-36.patch3.13 KB
new interdiff_34-36.txt730 bytes

Trying to fix the error and attaching the interadiff file

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.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. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.