Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add LegacyContext::getLegacyAdminLink #11309

Merged
merged 3 commits into from Nov 15, 2018

Conversation

jolelievre
Copy link
Contributor

@jolelievre jolelievre commented Nov 7, 2018

Questions Answers
Branch? 1.7.5.x
Description? The import feature looped indefinitely because it tried to redirect to the legacy controller, but because of the _legacy_link feature it was redirected to itself. So a new method is added in LegacyContext to be able to build a legacy link.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #10838
How to test? See #10838

This change is Reviewable

@jolelievre jolelievre added this to the 1.7.5.0 milestone Nov 7, 2018
@prestonBot prestonBot added 1.7.5.x Branch Bug Type: Bug labels Nov 7, 2018
@@ -125,7 +125,8 @@ public function importAction(Request $request)
}

$formData = $form->getData();
if (!$errors = $formHandler->save($formData)) {
$errors = $formHandler->save($formData);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

*
* @return string
*/
public function getLegacyAdminLink($controller, $withToken = true, $extraParams = array())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this should go into LegacyContext class, rather than in Link core class ?

As it is inspired by https://github.com/PrestaShop/PrestaShop/blob/develop/classes/Link.php#L807

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, but then it should be in both no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matks done! ;)

@Quetzacoalt91 Quetzacoalt91 added the Waiting for QA Status: action required, waiting for test feedback label Nov 15, 2018
@marionf marionf self-assigned this Nov 15, 2018
@marionf marionf added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Nov 15, 2018
@marionf marionf removed their assignment Nov 15, 2018
@Quetzacoalt91 Quetzacoalt91 merged commit 063479c into PrestaShop:1.7.5.x Nov 15, 2018
@Quetzacoalt91
Copy link
Member

Thank you @jolelievre

@jolelievre jolelievre deleted the import-loop branch November 27, 2018 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.5.x Branch Bug Type: Bug QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants