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 hooks when they already exist #669

Merged
merged 2 commits into from
Feb 26, 2024

Conversation

Hlavtox
Copy link
Contributor

@Hlavtox Hlavtox commented Feb 7, 2024

Questions Answers
Description? Improves inserting of hooks into database during update. More info below.
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket?
Sponsor company
How to test?

Information

  • Right now, if an hook exists in a database via an automatic creation, it has no title and description.
  • When you upgrade, it runs INSERT IGNORE, which doesn't result in an error, but it just doesn't do anything.
  • Much better solution is to update the key if found and not use IGNORE.
  • Yes, it's safe. UNIQUE KEY hook_name (name) exists in 1.6, maybe even sooner.

@Hlavtox Hlavtox added this to the 5.0.1 milestone Feb 7, 2024
@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 7, 2024

Ping @kpodemski @ShaiMagal

ShaiMagal
ShaiMagal previously approved these changes Feb 8, 2024
jolelievre
jolelievre previously approved these changes Feb 8, 2024
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @Hlavtox

boherm
boherm previously approved these changes Feb 14, 2024
@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 20, 2024

@PrestaShop/qa-functional Guys can I get a QA on this pls?

@Hlavtox Hlavtox dismissed stale reviews from boherm, jolelievre, and ShaiMagal via 815fb7f February 20, 2024 14:20
@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 20, 2024

@matks @jolelievre Please reapprove, I needed to rebase it. It would be good if you could test it, so we don't loose time. Just throw it into phpmyadmin and run it multiple times.

jolelievre
jolelievre previously approved these changes Feb 20, 2024
@MatShir
Copy link

MatShir commented Feb 20, 2024

I guess it would be QA by dev. But could you clarify the steps to reproduce the bugs? 🙏
The test would be done by someone else than the PR's author.

@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 21, 2024

@MatShir Every dev will understand what the PR means and how to test it. :-)

@MatShir
Copy link

MatShir commented Feb 21, 2024

😩 if you say so !

@jolelievre
Copy link
Contributor

I tested the queries manually and it seems to work perfectly. Just waiting for the CI to be green and we can merge this one.

@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 23, 2024

Thank you @jolelievre 🚀 The CI is broken on all PRs, the 8.0.0 always fails 🤷‍♂️ Ping @nesrineabdmouleh @Progi1984

jolelievre
jolelievre previously approved these changes Feb 23, 2024
@kpodemski
Copy link
Contributor

go go go @jolelievre you got this 😂

@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 23, 2024

@jolelievre Still frozen :(

@jolelievre
Copy link
Contributor

The CI for 8.0 is still red but it will be fixed in another PR #673

@Hlavtox Hlavtox merged commit 4965d1e into PrestaShop:dev Feb 26, 2024
92 of 100 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants