I found this bad practice in the OpmlFeedAdd.php

public function submitForm(array &$form, FormStateInterface $form_state) {
    $validators = array('file_validate_extensions' => array('opml xml'));
    if ($file = file_save_upload('upload', $validators, FALSE, 0)) {
      $data = file_get_contents($file->getFileUri());
    }

## Proposed change

Add in the field the validator and check it in the validateForm

CommentFileSizeAuthor
#8 2702475-8.patch1.17 KBSaphyel
#7 2702475-7.patch770 bytesSaphyel
#7 interdiff_2702475_5-7.patch770 bytesSaphyel
#5 2702475-5.patch538 bytesfelribeiro
#3 2702475-3.patch544 bytesfelribeiro
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Saphyel created an issue. See original summary.

felribeiro’s picture

felribeiro CreditAttribution: felribeiro at CI&T commented
Assigned: Unassigned » felribeiro
felribeiro’s picture

felribeiro CreditAttribution: felribeiro at CI&T commented
Assigned: felribeiro » Unassigned
Status: Active » Needs review
FileSize
2702475-3.patch544 bytes

I added the field validator, but I don't know about the validador.
@Saphyel, your suggestion is to bring all this code to the form validation?

    $validators = array('file_validate_extensions' => array('opml xml'));
    if ($file = file_save_upload('upload', $validators, FALSE, 0)) {
      $data = file_get_contents($file->getFileUri());
    }
    else {
      // @todo Move this to a fetcher implementation.
      try {
        $response = $this->httpClient->get($form_state->getValue('remote'));
        $data = (string) $response->getBody();
      }
      catch (RequestException $e) {
        $this->logger('aggregator')->warning('Failed to download OPML file due to "%error".', array('%error' => $e->getMessage()));
        drupal_set_message($this->t('Failed to download OPML file due to "%error".', array('%error' => $e->getMessage())));
        return;
      }
    }
joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented
Status: Needs review » Needs work 
+++ b/core/modules/aggregator/src/Form/OpmlFeedAdd.php	2016-04-08 14:52:33.074099168 -0300
@@ -75,6 +75,9 @@
+        'file_validate_extensions' => array('opml xml'),
+      ),      ¶
     );

white space

felribeiro’s picture

felribeiro CreditAttribution: felribeiro at CI&T commented
Status: Needs work » Needs review
FileSize
2702475-5.patch538 bytes
Saphyel’s picture

Saphyel CreditAttribution: Saphyel as a volunteer commented
Assigned: Unassigned » Saphyel
Status: Needs review » Needs work
Issue tags: +DCTransylvania

felribeiro well done! I'm gonna complete it

Saphyel’s picture

Saphyel CreditAttribution: Saphyel as a volunteer commented
Assigned: Saphyel » Unassigned
Status: Needs work » Needs review
FileSize
interdiff_2702475_5-7.patch770 bytes
2702475-7.patch770 bytes

Also this can be cherry-pick to 8.0.x or 8.2.x

Saphyel’s picture

Saphyel CreditAttribution: Saphyel as a volunteer commented
FileSize
2702475-8.patch1.17 KB

wrong file

The last submitted patch, 7: 2702475-7.patch, failed testing.

The last submitted patch, 7: interdiff_2702475_5-7.patch, failed testing.

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.

dpacassi’s picture

dpacassi
Primary language English
Location Zürich, Switzerland
CreditAttribution: dpacassi at Liip for FREITAG lab. AG commented
Issue tags: -Novice

Removing the Novice tag as the code changes seem to be quite advanced.
There's someone with experience needed to review this patch.

dpacassi’s picture

dpacassi
Primary language English
Location Zürich, Switzerland
CreditAttribution: dpacassi at Liip for FREITAG lab. AG commented
Title: Improve form code » Move #upload_validators to buildForm()

Updating the issue title to something more specific.

mmrares’s picture

mmrares CreditAttribution: mmrares commented
Issue tags: +Needs issue summary update

Please provide a better summary on why this is a bad practice.

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
quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Project: Drupal core » Aggregator
Version: 9.3.x-dev » 1.x-dev
Component: aggregator.module » Code

The aggregator module has been removed from Core in 10.0.x-dev and now lives on as a contrib module. Issues in the Core queue about the aggregator module, like this one, have been moved to the contrib module queue.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Needs review » Closed (won't fix)

We're in 'keep the lights on mode' now this is out of core.

You probably don't want to upload a file in a validation handler, in case another handler indicates it was invalid.

I think validating file uploads in submit handlers is a pretty wide-spread practice.