Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Working on this

tim.plunkett’s picture

Status: Active » Needs review
FileSize
24.72 KB

Status: Needs review » Needs work

The last submitted patch, 2: file-2326875-2.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
24.72 KB

Rerolled for now without the hook_element_info() removal in light of #2326409: Annotate render element plugins

Status: Needs review » Needs work

The last submitted patch, 4: 2326875-file-4.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
24.78 KB
526 bytes

Status: Needs review » Needs work

The last submitted patch, 6: 2326875-file-6.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
25.83 KB
1.05 KB

file_managed_file_validate() was used in one other place.

Status: Needs review » Needs work

The last submitted patch, 8: 2326875-file-8.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
28.58 KB
6.33 KB
jibran’s picture

+++ b/core/modules/file/src/Element/ManagedFile.php
@@ -0,0 +1,350 @@
+            if ($file = file_load($fid)) {

We can use static method here.

tim.plunkett’s picture

FileSize
28.28 KB

Okay

The last submitted patch, 10: 2326875-file-10.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: 2326875-file-12.patch, failed testing.

tim.plunkett’s picture

FileSize
527 bytes
28.36 KB

Duh! We need to specify the #value_callback until its a proper callback.

tim.plunkett’s picture

Status: Needs work » Needs review
jibran’s picture

Status: Needs review » Needs work

If you disagree with 1 and 2 it's fine by me perhaps we can add @todo (with issue id) for those methods.

  1. +++ b/core/modules/file/src/Element/ManagedFile.php
    @@ -0,0 +1,348 @@
    +      if ($files = file_managed_file_save_upload($element, $form_state)) {
    

    IMO we can also move this function here.

  2. +++ b/core/modules/file/src/Element/ManagedFile.php
    @@ -0,0 +1,348 @@
    +      '#submit' => ['file_managed_file_submit'],
    ...
    +      '#submit' => ['file_managed_file_submit'],
    

    IMO we have to move this function as well to this element file. Right know we are not using it at anyplace other then this.

  3. +++ b/core/modules/file/src/Element/ManagedFile.php
    @@ -0,0 +1,348 @@
    +      $element['upload_button']['#ajax']['progress']['path'] = 'file/progress/' . $upload_progress_key;
    

    Can't we use controller/routes here?

  4. +++ b/core/modules/file/src/Element/ManagedFile.php
    @@ -0,0 +1,348 @@
    +   * of in file_managed_file_process(), because #access for these buttons
    ...
    +   * form_builder() for more detailed information about the relationship between
    

    Minor doc update file_managed_file_process doesn't exist anymore and from_builder is deprecated. There is one more ref to file_managed_file_process just above file_managed_file_submit we have to update that as well.

iMiksu’s picture

FileSize
2.28 KB
28.78 KB

This should resolve #17.4.

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: 2326875-file-18.patch, failed testing.

Status: Needs work » Needs review

jibran queued 18: 2326875-file-18.patch for re-testing.

jibran’s picture

+++ b/core/modules/file/file.module
@@ -52,14 +52,22 @@ function file_help($route_name, RouteMatchInterface $route_match) {
+ * @todo Remove once https://www.drupal.org/node/2326409 is in.

this issue is in so needs reroll.

tim.plunkett’s picture

FileSize
28.66 KB
1.9 KB
jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thank you married guy :D

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 69be11e and pushed to 8.0.x. Thanks!

  • alexpott committed 69be11e on 8.0.x
    Issue #2326875 by tim.plunkett, iMiksu | almaudoh: Convert...

Status: Fixed » Closed (fixed)

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