Comments

tim.plunkett’s picture

StatusFileSize
new6.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
StatusFileSize
new6.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

StatusFileSize
new80.99 KB
tim.plunkett’s picture

Title: Replace $form_state['redirect'] and $form_state['redirect_route'] with setRedirect() » Replace $form_state['redirect_route'] with setRedirect()
StatusFileSize
new109.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
StatusFileSize
new109.85 KB
new4.14 KB

Fixed a couple things.

tim.plunkett’s picture

StatusFileSize
new109.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
StatusFileSize
new111.75 KB
new2.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.