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

Use PHP constants for doctrine config #36840

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

matks
Copy link
Contributor

@matks matks commented Sep 6, 2024

Questions Answers
Branch? 8.2.x
Description? Use PHP constants for doctrine config as informations collected in #33524 indicate the value of PDO::MYSQL_ATTR_MULTI_STATEMENTS change on different environments . This PR is targeting 8.2.x instead of develop and replaces #36732
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
How to test? Please see below
UI Tests I will add the UI tests when I have fixed the CI
Fixed issue or discussion? Should fix #33524
Related PRs
Sponsor company

How to test

As this is a config specific item, the behavior of the shop should be unchanged. To verify the item, a developer could do:

  1. Take develop branch and insert the below 2 lines in a Symfony controller to be able to verify the value of the configuration options passed to Doctrine
  2. Take this Pull Request branch and do the same, verify that despite the code changes the configuration options passed to Doctrine remain unchanged

2 lines of code

$b = $this->get('doctrine.dbal.default_connection');
dump($b->getParams());

The behavior is "nothing has changed" because I believe the fix will only fix the situation for a subset of users, victims of #33524

@matks matks requested a review from a team as a code owner September 6, 2024 08:27
@ps-jarvis ps-jarvis added develop Branch Bug fix Type: Bug fix labels Sep 6, 2024
@ps-jarvis ps-jarvis added 8.2.x and removed develop Branch labels Sep 6, 2024
Copy link
Contributor

@marsaldev marsaldev left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Sep 9, 2024
@kpodemski
Copy link
Contributor

@PrestaShop/qa-functional no functional tests are needed in my opinion, installation process is tested multiple times in automated tests

@florine2623 florine2623 added the Waiting for dev Status: action required, waiting for tech feedback label Sep 10, 2024
@florine2623
Copy link
Contributor

Ok @kpodemski , I agree :)

@kpodemski kpodemski merged commit e0dc838 into PrestaShop:8.2.x Sep 10, 2024
38 checks passed
@ps-jarvis
Copy link

PR merged, well done!

Message to @PrestaShop/committers: do not forget to milestone it before the merge.

@kpodemski
Copy link
Contributor

thank you @matks !

@kpodemski kpodemski added this to the 8.2.0 milestone Sep 10, 2024
@kpodemski kpodemski removed Waiting for QA Status: action required, waiting for test feedback Waiting for dev Status: action required, waiting for tech feedback labels Sep 10, 2024
@matks matks deleted the fix-doctrine-const branch September 10, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.2.x Bug fix Type: Bug fix
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants