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

Stop using deprecated hook #32

Merged
merged 6 commits into from Jan 10, 2024
Merged

Stop using deprecated hook #32

merged 6 commits into from Jan 10, 2024

Conversation

metacreo
Copy link
Contributor

@metacreo metacreo commented Sep 21, 2023

Questions Answers
Description? Module is using an outdated hook. Using an alias throws a deprecated message in logs. The hook "createAccount" is deprecated, please use "actionCustomerAccountAdd" instead in module "statsdata".
Type? refacto
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? Please indicate how to best verify that this PR is correct.

Example of errors

[proxy_fcgi:error] [pid 29439:tid 35444818688] [remote 0.0.0.0:9822] AH01071:
Got error 'PHP message: PHP Deprecated: 
The hook "createAccount" is deprecated, please use "actionCustomerAccountAdd" instead in module "statsdata". in /usr/local/www/mysite.com/www/classes/Hook.php on line 914

The hook "createAccount" is deprecated, please use "actionCustomerAccountAdd" instead in module "statsdata".
@kpodemski
Copy link
Contributor

Hello @metacreo

It makes sense, but you'd need to unregister the old hook, and register the new one in the upgrade file like this:
https://github.com/PrestaShop/statsdata/blob/dev/upgrade/upgrade-2.1.1.php

The next version will be 2.1.2

@watou
Copy link

watou commented Sep 25, 2023

What would happen if a 1.7 user were to upgrade to this? Do your changes have to be made sensitive to pre-PS8 users?

@metacreo
Copy link
Contributor Author

@watou I tested on 1.7.8.10. Nothing bad. Module steel work.

@watou
Copy link

watou commented Sep 27, 2023

@watou I tested on 1.7.8.10. Nothing bad. Module steel work.

That's great! My only point was, is the version of PrestaShop that introduced theactionCustomerAccountAdd hook later than 1.7.1.0, because this proposed change could have a problem on previous supported versions.

@metacreo
Copy link
Contributor Author

I am dragging my resource from the 1.6.1.24 branch with all the modules and my own modules.
The transition to version 8 took me 1 day and even production stopped for only 5 minutes. Sooner or later you will have to drop old versions.

@watou
Copy link

watou commented Sep 27, 2023

Looking further it appears it's only a PrestaShop 8 issue that the deprecated notice is printed when hook aliases are used, so your change to set the min PS version to 1.7.8.10 is not needed. Your previous code change should be good for all PS versions since 1.5.

@metacreo
Copy link
Contributor Author

Basically this is good; should have gotten rid of aliases earlier. Moreover, now there is a description of hooks in the docs.

@florine2623
Copy link
Contributor

Where are the steps to reproduce ?
Is this supposed to be tested by a dev ?

@metacreo
Copy link
Contributor Author

metacreo commented Oct 6, 2023

To reproduce this need read top of this page. ;)
#32 (comment)

@florine2623
Copy link
Contributor

@metacreo ,

I don't have the error "Got error 'PHP message: PHP Deprecated: The hook "createAccount" is deprecated, please use "actionCustomerAccountAdd" instead in module "statsdata"." in my log file.

@metacreo
Copy link
Contributor Author

@metacreo ,

I don't have the error "Got error 'PHP message: PHP Deprecated: The hook "createAccount" is deprecated, please use "actionCustomerAccountAdd" instead in module "statsdata"." in my log file.

@florine2623 Nice. What PHP version you use? What is value of 'error_reporting' param in your php.ini file? Have you professional php config or just as is? If you not see deprecation error it means your php not configured to show it or php version is older.

@Hlavtox Hlavtox changed the title Update statsdata.php Stop using deprecated hook Jan 3, 2024
Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

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

Hi @metacreo, the change is legit, we can merge this. :-) I checked the lowest supported version in this module - 1.7.1.0 and there is already the new hook in that core version.

I also updated the description and PR name.

@Hlavtox Hlavtox added this to the 2.1.2 milestone Jan 3, 2024
@florine2623 florine2623 self-assigned this Jan 4, 2024
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.

Hello @metacreo ,

I tested your PR.

What PHP version you use? What is value of 'error_reporting' param in your php.ini file? Have you professional php config or just as is?

I'm using PHP 8.1.13.
In my php.ini file, I haven't changed anything. I'm using my localhost to test. Here's what I have :
error_reporting = MAMP_error_reporting_MAMP

Update to 2.1.2 is successful✅

Screenshot 2024-01-04 at 14 11 08

But here's what I have hooked to the module statsdata :

Screenshot 2024-01-04 at 14 06 44

It is not what's expected from the PR.
Screenshot 2024-01-04 at 14 06 53

I have cleared cache from browser and PS.
Am I doing something wrong ? 😭

@florine2623 florine2623 removed their assignment Jan 4, 2024
@metacreo
Copy link
Contributor Author

@florine2623 Hello,
My PHP version 8.2.10.
Well, I suspect that you have a lot of PHP errors, but you will never see them. You are not using production PHP.INI, your PHP is not configured, or rather configured to hide compiler errors.

You are using MAMP, which has never been used on production servers. This is extremely unprofessional. Production servers usually run on Linux with PHP-FPM.

"In my php.ini file, I haven't changed anything.", "Am I doing something wrong ?"

; Common Values:
;   E_ALL (Show all errors, warnings and notices including coding standards.)
;   E_ALL & ~E_NOTICE  (Show all errors, except for notices)
;   E_ALL & ~E_NOTICE & ~E_STRICT  (Show all errors, except for notices and coding standards warnings.)
;   E_COMPILE_ERROR|E_RECOVERABLE_ERROR|E_ERROR|E_CORE_ERROR  (Show only errors)
; Default Value: E_ALL
; Development Value: E_ALL
; Production Value: E_ALL & ~E_DEPRECATED & ~E_STRICT
; https://php.net/error-reporting
error_reporting = E_ALL & ~E_DEPRECATED & ~E_STRICT

For testing you must use error_reporting = E_ALL

Hooks not changed, because you need clean PS cache before update module. function upgrade_module_2_1_2 not works because in fact you install same module 2.1.1 from ps repo.

@metacreo
Copy link
Contributor Author

@florine2623 After update check ps_module table. Last record must be statsdata 1 2.1.2
statsdata.php and config.xml must have $this->version = '2.1.2'; and <version><![CDATA[2.1.2]]></version> values.
Just now tested again on my server, all works as expected.

@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 10, 2024

@metacreo Error reporting levels is overridem from the app side. @florine2623 has the environment correctly set.

And also, it shoud NOT be required to delete cache before upgrading, it has to work.

@florine2623 I had some issues with upgrading modules, could you try it on 8.1.3, so we know if it's a core issue? :-)

@florine2623 florine2623 self-assigned this Jan 10, 2024
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.

Hello @metacreo @Hlavtox ,

Works as expected ^^
Screenshot 2024-01-10 at 10 49 11

It is QA ✅

@kpodemski kpodemski merged commit a171d63 into PrestaShop:dev Jan 10, 2024
13 checks passed
@kpodemski
Copy link
Contributor

thanks @metacreo

@metacreo
Copy link
Contributor Author

@Hlavtox

Error reporting levels is overridem from the app side. @florine2623 has the environment correctly set.

In DEV mode only ... with true PS_DISPLAY_COMPATIBILITY_WARNING

And also, it shoud NOT be required to delete cache before upgrading, it has to work.

It required to delete cache before upgrading if you want manually init update from update TAB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
9 participants