We have to convert the module to short array syntax as per new coding standards.

Comments

deepakkumar14 created an issue. See original summary.

deepakkumar14’s picture

Status: Needs work » Needs review
StatusFileSize
new73.01 KB

Submitting patch fro this.

nkoporec’s picture

Status: Needs review » Needs work

Found two missed array().
1. In RedirectAPITest.php

  /**
   * Test redirect_parse_url().
   */
  public function testParseURL() {
    //$test_cases = array(
    //  array(
    //    'input' => array('b' => 'aa', 'c' => array('c2' => 'aa', 'c1' => 'aa'), 'a' => 'aa'),
    //    'expected' => array('a' => 'aa', 'b' => 'aa', 'c' => array('c1' => 'aa', 'c2' => 'aa')),
    //  ),
    //);
    //foreach ($test_cases as $index => $test_case) {
    //  $output = redirect_parse_url($test_case['input']);
    //  $this->assertIdentical($output, $test_case['expected']);
    //}
  }

2. In RedirectCheckerTest.php

//    $request = $this->getRequestStub('index.php', 'GET',
//      array(RouteObjectInterface::ROUTE_OBJECT => $route));
//    $this->assertFalse($checker->canRedirect($request), 'Cannot redirect if we are requesting a admin path');
//
//    // We are at admin path with ignore_admin_path set to TRUE.
//    $config['redirect.settings']['ignore_admin_path'] = TRUE;
//    $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state);
//
//    $request = $this->getRequestStub('index.php', 'GET',
//      array(RouteObjectInterface::ROUTE_OBJECT => $route));
//    $this->assertTrue($checker->canRedirect($request), 'Can redirect a admin with ignore_admin_path set to TRUE');
prashant.c’s picture

Status: Needs work » Needs review

@nkoporec

This is commented out code why do we need to make the changes in this ?

berdir’s picture

Status: Needs review » Needs work

This has several conflicts now and needs a reroll, sorry for the delay.

daiwik.addweb’s picture

StatusFileSize
new70.65 KB

@all Please find my patch where I resolved it from the first & worked on it as per the standard errors are shown in Pareview. kindly review it & let me know if you still face any challenges and issues.

daiwik.addweb’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: redirect-2961293-6.patch, failed testing. View results

thalles’s picture

Status: Needs work » Needs review
StatusFileSize
new73.65 KB

Follow the patch!

dww’s picture

Status: Needs review » Reviewed & tested by the community

There are a few remaining references to array( with patch #9 applied, but they're all in commented-out code. I guess we don't care about that, and hopefully we'll fix any remaining CS bugs as we touch those parts.

This is blocking a lot of other things. It's a huge patch, and a beast to review. I just went over it all and didn't catch any unrelated or unintended changes. The only thing I saw that looked at all funny is this (from src/Form/RedirectForm.php):

        $form_state->setErrorByName('redirect_source', $this->t('The source path\
 %source is already being redirected. Do you want to <a href="@edit-page">edit t\
he existing redirect</a>?',
          [
            '%source' => $source['path'],
            '@edit-page' => $redirect->url('edit-form')]));

I'm not totally sure that's legit code-style, but that's basically identical to what we have now, with only the array() vs. [] change. So if that's not legit, the code in Git isn't legit, either. So it's not a regression in code style because of this patch. If that needs further CS help, it should probably happen in a follow-up or as part of #2957751: Maintaining drupal coding standards.

Applies cleanly to the end of 8.x-1.x.

All tests pass with the patch applied.

Brief local click-testing seems to be fine, too.

Let's get this in so we don't keep running into people trying to fix any of this in unrelated patches (like at #2958635-6: Restore link to add a redirect on node edit forms).

Yes, it's going to make D7 patches no longer apply to D8. But I think the era of "easy" backporting/cherrypicking is probably over, and anything that touches D7 at this point will need to be rerolled for D8, anyway.

Given all of this, RTBC.

Thanks,
-Derek

  • Berdir committed e61ef3d on 8.x-1.x authored by thalles
    Issue #2961293 by deepakkumar14, RoshniPatel.addweb, thalles, dww:...
berdir’s picture

Status: Reviewed & tested by the community » Fixed

> Yes, it's going to make D7 patches no longer apply to D8. But I think the era of "easy" backporting/cherrypicking is probably over.

That era never existed, there's hardly a line of code the same as in the 7.x version. I don't care about 7.x but this will break a lot of other contrib patches, but there's not too much activity atm.

dww’s picture

Thanks for getting this in!

I don't care about 7.x

I mostly don't, either, but I'm concerned about the state of this module. It seems the D7 and D8 versions have completely diverged, and no one is paying any attention to that fact. See for example #2220061-11: Add permission to restrict access to a user's own redirects only. Also, I just tried skimming #905914: Merge global redirect functions into Redirect module (linked from the project page) which is a scary and confusing read about yet more divergent functionality between the D7 and D8 versions. This seems to be evolving into quite a mess. Is there any plan to deal with this problem and bring some sanity and unification to this? Or should we fork the D8 project into a completely separate git repo and project? If the D7 and D8 branches are complete rewrites, maintained by different people, with different standards, and different visions, with diverging feature sets, why share a project page? It seems that's going to create more problems than it solves.

Thanks,
-Derek

Status: Fixed » Closed (fixed)

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