Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

FileSize
6.03 KB

Here's an example patch to kick things off. This does one of each kinds of redirects in HEAD I think.

Not sending to the bot until the Url issue goes in.

tim.plunkett’s picture

Status: Active » Needs review

Crell queued 1: redirect-2189661-1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: redirect-2189661-1.patch, failed testing.

tim.plunkett’s picture

Issue tags: +FormState
jibran’s picture

Can we change the scope of the issue to Add FormState::setRedirect()?

tim.plunkett’s picture

We already have setRedirect(\Drupal\Core\Url $url);

jibran’s picture

So perhaps replace $form_state['redirect'] and $form_state['redirect_route'] with setRedirect(\Drupal\Core\Url $url);. I think it will include conversion in it.

tim.plunkett’s picture

There was also discussion of a method that took ($route_name, $route_parameters, $options) as params.

setRedirectUrl() and setRediect()?

tim.plunkett’s picture

Title: Convert $form_state['redirect'] and $form_state['redirect_route'] to use \Drupal\Core\Url » Replace $form_state['redirect'] and $form_state['redirect_route'] with setRedirect()
Status: Needs work » Needs review
FileSize
6.45 KB

I think this is what @Berdir was describing in IRC.

jibran’s picture

I like this apporoch. Are these all the redirect we have in the core?
Minor doc improvement suggestions.

  1. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
    @@ -75,6 +75,23 @@ public function setFormState(array $form_state_additions);
    +   * @see \Drupal\Core\Form\FormSubmitterInterface::redirectForm()
    

    I think we should also add @see setRedirectUrl here.

  2. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
    @@ -84,7 +101,7 @@ public function setIfNotExists($property, $value);
        * @see \Drupal\Core\Form\FormSubmitterInterface::redirectForm()
    

    I think we should also add @see setRedirect here.

tim.plunkett’s picture

No, there are about 225 more to convert. I just wanted to get the API right before converting them all.

Berdir’s picture

Yep, that's exactly what I meant, nice.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Going to work on this on my flight.

tim.plunkett’s picture

Just posting what I got through so far.

tim.plunkett’s picture

tim.plunkett’s picture

Title: Replace $form_state['redirect'] and $form_state['redirect_route'] with setRedirect() » Replace $form_state['redirect_route'] with setRedirect()
FileSize
109.16 KB

Hmm, I don't know if finishing all of the individual path-based redirects is in scope here. Those have been handled in dedicated issues.

The last submitted patch, 16: redirect_route-2189661-15.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 17: redirect_route-2189661-17.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
109.85 KB
4.14 KB

Fixed a couple things.

tim.plunkett’s picture

FileSize
109.91 KB

Rerolled.

jibran’s picture

Thank you for the reroll. This is RTBC. Needs change notice update and a follow up for path-based redirects.

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

This is looking good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

FormState::getRedirect() has the following code / comment:

    if ($redirect_route = $this->get('redirect_route')) {
      // @todo Remove once all redirects are converted to \Drupal\Core\Url. See
      //   https://www.drupal.org/node/2189661.
      if (!($redirect_route instanceof Url)) {
        $redirect_route += array(
          'route_parameters' => array(),
          'options' => array(),
        );
        $redirect_route = new Url($redirect_route['route_name'], $redirect_route['route_parameters'], $redirect_route['options']);
      }

      $redirect_route->setAbsolute();
      return $redirect_route;
    }

At the very least the @todo needs updating to point to a new issue.

alexpott’s picture

@jibran re #22 can you list the CRs you think need updating. Thanks.

tim.plunkett’s picture

Status: Needs work » Needs review
Related issues: +#2315807: Remove support for path-based form redirects
FileSize
111.75 KB
2.05 KB

We can actually remove that right now, the string-based stuff will be covered in this follow-up.

jibran’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 627a719 and pushed to 8.0.x. Thanks!

  • alexpott committed 627a719 on 8.0.x
    Issue #2189661 by tim.plunkett: Replace $form_state['redirect_route']...
jibran’s picture

Thank you @alexpott for the commit. I have updated the change notices.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Thanks!

Status: Fixed » Closed (fixed)

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