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 a paragraph in 1.7.5 changes page about the _legacy_link #168

Merged
merged 6 commits into from Dec 11, 2018

Conversation

Projects
None yet
5 participants
@jolelievre
Copy link
Contributor

jolelievre commented Nov 28, 2018

No description provided.

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Dec 4, 2018

To sum up: are the docs "correct" with the behavior in the code?

If it does, I'm in favor of the merge: WDYT?

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 5, 2018

the doc show what I changed in the core 1.7 code to make getAdminLink work correctly, in this context it does work
but indeed it won't work in 1.6 so the question is what do we advise to developers who need to make module compatible in both 1.6 and 1.7
either we provide an example for each version (the one you suggested) or we update the 1.6 so that getAdminLink is able to deal with these extra parameters but we are extending the scope of 1.6 then and I'm not sure we want to do that

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 5, 2018

@mickaelandrieu I tried what you suggested in a 1.6.1 version with php 7.1:

link->getAdminLink('AdminOrders', true, [], ['id_customer' => $customer->id|intval, 'viewcustomer' => 1]) . '&id_order={$order->id|intval}&vieworder';

it seems to work (no errors nor notices), I think I'm gonna add this as a suggestion for 1.6 developers

@jolelievre jolelievre force-pushed the jolelievre:link-changes branch from 2d7cd95 to 05f171e Dec 5, 2018

@eternoendless

This comment has been minimized.

Copy link
Member

eternoendless commented Dec 7, 2018

@jolelievre I took the liberty of rephrasing things a little. If you are OK with the content I'm in favor of merging this PR.

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 7, 2018

@eternoendless you did right, it's clearer this way It's ok for me too
Did you see my comment though? #168 (comment)
Do you think it's something we could introduce?
too late for 1.7.5? worth considering for 1.7.6? what about 1.6.1?

@jolelievre jolelievre force-pushed the jolelievre:link-changes branch from 8a75d7f to 7375776 Dec 11, 2018

Update src/content/1.7/modules/core_updates/1.7.5.md
Fix wording

Co-Authored-By: jolelievre <jo.lelievre@gmail.com>
@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 11, 2018

@Quetzacoalt91 good to go?

@jolelievre jolelievre merged commit 5bfbedc into PrestaShop:master Dec 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment