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

Fixed access to Front Office container from modules #9226

Merged
merged 2 commits into from Jun 28, 2018

Conversation

Projects
None yet
4 participants
@123monsite-regis
Contributor

123monsite-regis commented Jun 27, 2018

Questions Answers
Branch? 1.7.4.x
Description? Accessing to Symfony services from modules hooks can be broken.
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? -
How to test? Please see below
  • use must test whith module provided on #8922 by Mickael
    and adding some case where there is no controller like :
 public function hookActionProductUpdate($params)
{
  [...]
  $request = $this->get('request');
  [...]
}
  • please see discussion on #8922
  • note , that as indicades on the php comments " To be removed - because useless - when the migration will be done. "
  • extra note : sorry use case are so various that i does not know how to test alls.

This change is Reviewable

@prestonBot prestonBot added the 1.7.4.x label Jun 27, 2018

@@ -3185,7 +3185,7 @@ public function isAdminLegacyContext()
*/
public function isSymfonyContext()
{
return !defined('ADMIN_LEGACY_CONTEXT') && $this->context->controller instanceof AdminLegacyLayoutControllerCore;
return !defined('ADMIN_LEGACY_CONTEXT') && defined('_PS_ADMIN_DIR_');

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 27, 2018

Contributor

should be even better with return !$this->isAdminLegacyContext() && defined('PS_ADMIN_DIR'); to remove duplicate :)

This comment has been minimized.

@123monsite-regis

123monsite-regis Jun 27, 2018

Contributor

edited

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 27, 2018

if it fixes the regression, its ok for me!

The faster we finish the migration, the better => end of the year if we got help :-)

@mickaelandrieu mickaelandrieu added the Bug label Jun 27, 2018

@mickaelandrieu mickaelandrieu changed the title from fix Fixed access to Front Office container from modules to Fixed access to Front Office container from modules Jun 27, 2018

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 28, 2018

@mickaelandrieu should it need a Qa review?

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 28, 2018

I guess yes, but from a developer: would you mind to install the module, add some hooks from Product pages (from both BO or BO) et check if we have always access to a service container?

@PierreRambaud PierreRambaud modified the milestones: 1.7.4.0, 1.7.4.1 Jun 28, 2018

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 28, 2018

@mickaelandrieu Just tried, LGTM

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 28, 2018

Thank you for the review @PierreRambaud, don't you think we should merge it in 1.7.4.0?

I just noticed you have changed the milestone ;)

@PierreRambaud PierreRambaud modified the milestones: 1.7.4.1, 1.7.4.0 Jun 28, 2018

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 28, 2018

Oups my bad, miss click while watching milestone :/ Sure we can merge but we need to notify @eternoendless who's currently building a new release

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 28, 2018

ping @toutantic, could be really good to add this in 1.7.4.0 as it's a regression from 1.7.3.3 that have impacts on a big module.

Let us know 👍

@mickaelandrieu mickaelandrieu merged commit 104200f into PrestaShop:1.7.4.x Jun 28, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment