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

Update egulias/email-validator to 4.0.1 and bump multiple doctrine deps #33459

Merged
merged 5 commits into from Sep 27, 2023

Conversation

M0rgan01
Copy link
Contributor

@M0rgan01 M0rgan01 commented Jul 27, 2023

Questions Answers
Branch? develop
Description? Update egulias/email-validator to 4.0.1. This also requires updating:
doctrine/dbal from 2.13.8 to 3.6.5
doctrine/deprecations from 0.5.3 to 1.1.1
doctrine/lexer from 1.2.3 to 2.1.0
doctrine/orm from 2.12.1 to 2.14.3
Type? improvement
Category? CO
BC breaks? yes
Deprecations? no
How to test? CI and tests are green
UI tests https://github.com/M0rgan01/ga.tests.ui.pr/actions/runs/6313332809
Fixed ticket? #33445
Related PRs productcomments/178 PrestaShop/ps_linklist#179
Sponsor company -

Update details

Module prefix

We can no longer access module entities with a prefix like:

  • $entityManager->find('MyModule:Entity', $id);
  • $entityManager->createQuery('SELECT u FROM MyModule:Entity');

See doctrine/orm#8818

Exhaustive list of changes made following the DBAL doctrine update

  • Statement->execute() now return Result instant of Statement
  • Statement->closeCursor() is now Result->free()
  • Doctrine\DBAL\DBALException is now Doctrine\DBAL\Exception
  • QueryBuilder->setFirstResult() no longer allow null values (adding type)
  • Connection->getWrappedConnection()->inTransaction() move in Connection->getNativeConnection()->inTransaction()
  • Statement->execute() become:
    • Statement->executeQuery() for 'Select'
    • Statement->executeStatement() for 'Delete', 'Update' or 'Insert'
  • Result->fetchAll() is now Result->fetchAllAssociative()
  • Result->fetchAll(PDO::FETCH_COLUMN) is now Result->fetchFirstColumn()
  • Result->fetch() is now Result->fetchAssociative()
  • Result->fetch(PDO::FETCH_COLUMN) is now Result->fetchOne()

@M0rgan01 M0rgan01 requested a review from a team as a code owner July 27, 2023 15:10
@prestonBot prestonBot added develop Branch Bug fix Type: Bug fix labels Jul 27, 2023
nicosomb
nicosomb previously approved these changes Jul 27, 2023
@M0rgan01 M0rgan01 marked this pull request as draft July 27, 2023 15:35
@M0rgan01 M0rgan01 force-pushed the improvement/33445 branch 12 times, most recently from 07d4bd9 to 0633225 Compare July 31, 2023 12:08
@matks matks changed the title Update egulias/email-validator to 4.0.1 Update egulias/email-validator to 4.0.1 and bump multiple doctrine deps Jul 31, 2023
@prestonBot prestonBot added the Improvement Type: Improvement label Jul 31, 2023
@M0rgan01 M0rgan01 force-pushed the improvement/33445 branch 4 times, most recently from e5c0370 to 7a928ed Compare July 31, 2023 14:20
@@ -110,7 +110,7 @@ private function createAnnotationMappingDriver($moduleNamespace, $moduleEntityDi
$driverDefinition->addMethodCall('addExcludePaths', [[$indexFile]]);
}

return new DoctrineOrmMappingsPass($driverDefinition, [$moduleNamespace], [], false, [$modulePrefix => $moduleNamespace]);
return new DoctrineOrmMappingsPass($driverDefinition, [$moduleNamespace], [], false, []);
Copy link
Contributor Author

@M0rgan01 M0rgan01 Jul 31, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, so we can't define namespaces for modules' entities? Does it mean module also need to adapt how they use Doctrine entities? If so this will have to be documented in the BC breaks as well

Comment on lines -1090 to +1095
->setParameter(":$column", $value, $arrayType)
->setParameter($column, $value, $arrayType)
;
} else {
$qb
->andWhere("$column = :$column")
->setParameter(":$column", $value)
->setParameter($column, $value)
Copy link
Contributor Author

@M0rgan01 M0rgan01 Aug 2, 2023

Choose a reason for hiding this comment

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

I don't understand how it worked before. I had an error Named parameter "id_product" does not have a bound value. after update, in https://github.com/doctrine/dbal/blob/195aad31e9596e4093d417583f98342b6f0ec986/src/ArrayParameters/Exception/MissingNamedParameter.php#L11

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep the comma should only be added in the where, that was a mistake Maybe before Doctrine automatically understood it for some weird reason 😅

@hibatallahAouadni hibatallahAouadni self-assigned this Aug 31, 2023
Copy link
Contributor

@hibatallahAouadni hibatallahAouadni left a comment

Choose a reason for hiding this comment

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

Hello @M0rgan01

Thanks for your PR 🚀
Unfortunately, the auto tests are broken caused by this issue:
image

Thanks @Progi1984 for the screenshot 🙏
Here's the link: https://github.com/hibatallahAouadni/testing_pr/actions/runs/6042014563
Please check and feedback.

Thanks!

@hibatallahAouadni hibatallahAouadni added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Sep 1, 2023
@hibatallahAouadni hibatallahAouadni removed their assignment Sep 1, 2023
@M0rgan01 M0rgan01 added this to the 9.0.0 milestone Sep 4, 2023
@M0rgan01 M0rgan01 dismissed stale reviews from tleon and jolelievre via 60565c4 September 5, 2023 13:00
boherm
boherm previously approved these changes Sep 5, 2023
@jolelievre
Copy link
Contributor

jolelievre commented Sep 7, 2023

I created a PR that modified the UI tests a little PrestaShop/ga.tests.ui.pr#47
The purpose is to add two things:

Edit: an error in the dump export so new run here https://github.com/jolelievre/ga.tests.ui.pr/actions/runs/6108846839

boherm
boherm previously approved these changes Sep 19, 2023
@M0rgan01 M0rgan01 removed the Waiting for author Status: action required, waiting for author feedback label Sep 26, 2023
@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Sep 27, 2023
@jolelievre
Copy link
Contributor

@jolelievre jolelievre merged commit 2b2c8e3 into PrestaShop:develop Sep 27, 2023
18 checks passed
@MatShir MatShir added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC break Type: Introduces a backwards-incompatible break Bug fix Type: Bug fix develop Branch Improvement Type: Improvement Needs documentation Needs an update of the developer documentation QA ✔️ Status: check done, code approved
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet