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

Check if the hook is present before adding it #160

Merged
merged 2 commits into from Sep 18, 2023

Conversation

Hlavtox
Copy link
Contributor

@Hlavtox Hlavtox commented Mar 8, 2023

Questions Answers
Description? Checks if the sitemap hook is not present in the system, otherwise the adding would fail. The hook can be there from other modules, fixtures or my a failed uninstall. Also I removed the removal of the hook during uninstall - because you would lost all modules hooked into there, and they would not get rehooked.
Type? refacto
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? See below

➡️ Make sure to replace PREFIX_ in the queries with ps_ or whatever your prefix is.

How to test - scenario A

  1. Uninstall this module.
  2. Run following query in PHPMYADMIN. The hook may fail if you already have the hook there, but it's not a problem.
INSERT IGNORE INTO `PREFIX_hook` (`id_hook`, `name`, `title`, `description`, `position`) VALUES 
(NULL, 'gSitemapAppendUrls', 'GSitemap Append URLs', 'This hook allows a module to add URLs to a generated sitemap', '1');
  1. Verify the hook is there by running:
SELECT * FROM `PREFIX_hook` WHERE name = 'gSitemapAppendUrls';
  1. Install this module. It will work. It may not work without this PR.

How to test - scenario B

  1. Uninstall this module.
  2. Verify the hook stayed in the database by running. It was deleted before. With this PR, it stays.
SELECT * FROM `PREFIX_hook` WHERE name = 'gSitemapAppendUrls';

@Hlavtox Hlavtox added this to the 4.2.2 milestone Mar 8, 2023
Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

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

Hell @Hlavtox ,

Tested both scenarios on 1.7.8.8, 8.0.2, 8.1.x and develop with gsitemap v4.2.0 and v4.2.1.

The module is well installed after a simple uninstall and/or uninstall with option delete module folder on PS 1.7.8.8 and 8.0.2. ✅
Screenshot 2023-03-14 at 15 35 22

On PS 8.1.x and develop, it is NOK ❌
Screenshot 2023-03-14 at 15 37 40

Am I missing something ?
Thanks 🤗

@florine2623 florine2623 removed their assignment Mar 14, 2023
@lartist lartist removed this from the 4.3.0 milestone Apr 6, 2023
@Hlavtox Hlavtox added this to the 4.3.1 milestone Jun 14, 2023
@Hlavtox
Copy link
Contributor Author

Hlavtox commented Sep 1, 2023

@florine2623 Hi, can you retest it? It must have been some core issue, because I just tried it on 8.1.x and it works perfectly.

@Hlavtox
Copy link
Contributor Author

Hlavtox commented Sep 16, 2023

@PrestaShop/qa-functional Hi again, can you retest?

Copy link

@aniszr aniszr left a comment

Choose a reason for hiding this comment

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

Hello @Hlavtox

I tested your PR with both scenarios on 8.1.x & Develop :

- 8.1.x :
The module is well installed after a simple uninstall and/or uninstall with option delete module folder ✔️

image

The hook stayed in the database ✔️

image

- Develop:
The module is well installed after a simple uninstall and/or uninstall with option delete module folder ✔️

image

The hook stayed in the database ✔️
image

QA ✔️

Thanks!

@Hlavtox Hlavtox merged commit eb8a874 into PrestaShop:dev Sep 18, 2023
@Hlavtox Hlavtox deleted the add-hook-condition branch September 18, 2023 15:08
@aniszr aniszr self-assigned this Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
7 participants