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

SqlTranslator must throw a NotFoundResourceException #15515

Merged
merged 1 commit into from Sep 19, 2019

Conversation

@PierreRambaud
Copy link
Contributor

commented Sep 12, 2019

Questions Answers
Branch? 1.7.6.x
Description? The throwing exception isn't the good one if we check the documentation: https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Translation/Loader/LoaderInterface.php#L34
In case another exception is thrown, it crashs the application, whereas with the NotFoundResourceException, it's properly catch and the Translator knows what to do.
Missing const in console mode (DB_SERVER, DB_USER, etc), the config.inc.php must be included in console context.
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #15324
How to test? ./bin/console cache:warmup --env=prod must work

This change is Reviewable

@PierreRambaud PierreRambaud requested a review from PrestaShop/prestashop-core-developers as a code owner Sep 12, 2019
Copy link
Contributor

left a comment

one question

bin/console Show resolved Hide resolved
@PierreRambaud PierreRambaud force-pushed the PierreRambaud:fix/15324 branch from f9cfcd9 to 93543d3 Sep 12, 2019
Copy link
Member

left a comment

Looks good, but tests are failing for unrelated reasons

@PierreRambaud PierreRambaud force-pushed the PierreRambaud:fix/15324 branch from 93543d3 to 6d3e252 Sep 17, 2019
@PierreRambaud PierreRambaud force-pushed the PierreRambaud:fix/15324 branch from 6d3e252 to a71dda7 Sep 17, 2019
@PierreRambaud PierreRambaud reopened this Sep 18, 2019
@PierreRambaud PierreRambaud added this to the 1.7.6.2 milestone Sep 18, 2019
@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

./bin/console cache:warmup --env=prod must work

You can create a test for that => https://github.com/PrestaShop/PrestaShop/pull/15081/files#diff-a63019d16d68d746b3ddec7bbbc20e9eR40

Else, I won't trust you :trollface:

@PierreRambaud

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

./bin/console cache:warmup --env=prod must work

You can create a test for that => https://github.com/PrestaShop/PrestaShop/pull/15081/files#diff-a63019d16d68d746b3ddec7bbbc20e9eR40

Else, I won't trust you :trollface:

Not in this PR because I don't do the exact same change as you did 😅
I think your PR is more complete but this one is only to remove the regression :)

@matks
matks approved these changes Sep 19, 2019
@matks

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

Thank you @PierreRambaud

@matks matks merged commit ebe3a72 into PrestaShop:1.7.6.x Sep 19, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@PierreRambaud PierreRambaud deleted the PierreRambaud:fix/15324 branch Sep 19, 2019
@clotaire202

This comment has been minimized.

Copy link

commented Sep 24, 2019

Please note your PR can generate some trouble and BC for instance during module:install if a module already include config.inc.php even if this module is not yet installed.

@matks

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

Please note your PR can generate some trouble and BC for instance during module:install if a module already include config.inc.php even if this module is not yet installed.

You mean if the module uses require instead of require_once ?

Well that cant be helped unfortunately 😭we make sure we use require_once to avoid this if other scripts make good usage of require_once too

202-ecommerce added a commit to 202-ecommerce/PrestaShop that referenced this pull request Oct 22, 2019
according to PR PrestaShop#15515
202-ecommerce added a commit to 202-ecommerce/PrestaShop that referenced this pull request Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.