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

Validate smarty render calls (prevent errors from outdated themes) #34448

Merged

Conversation

ShaiMagal
Copy link
Contributor

@ShaiMagal ShaiMagal commented Nov 2, 2023

Questions Answers
Branch? 8.1.x
Description? Read below
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
How to test? You should have incorrect theme or add code above to file order-confirmation.tpl. Then you will receive error 500 with guest checkout. With this PR, error 500 dissapeared and working without problems.
UI Tests https://github.com/florine2623/testing_pr/actions/runs/6861890949
Fixed issue or discussion? I found, another e-shop owners had same issues with this: https://www.prestashop.com/forums/topic/1078118-error-500-php-fatal-error-uncaught-error-call-to-a-member-function-settemplate-on-null-in-configsmartyfrontconfigincphp97/
Related PRs
Sponsor company https://www.openservis.cz/

Description

Some "up to date" templates are targeted for PS 8.1, but throwing fatal errors. For example on order confirmation page due to guest conversion box, because some templates are using obsolete code from older versions PS... :/ (check code below)

This PR improving this behaviour. Because this take 2 hours of my life to investigate, what is happening there and for other people it can be useful. It is problem with theme developers, but PrestaShop must do the best to protect customers from bad themes as well. For end customer is very complicated to find, why they are receiving Fatal error 500 with guest order only and for any developers as well........

This PR can handle this problem and can protect eshop owners from outdated themes for example.

This is part of incorrect usage. This is reason for Fatal error 500 (line with "{render file=XXXX}".

  {block name='customer_registration_form'}
    {if $customer.is_guest}
      <div id="registration-form" class="card">
        <div class="card-block">
          <h4 class="h4">{l s='Save time on your next order, sign up now' d='Shop.Theme.Checkout'}</h4>
          {render file='customer/_partials/customer-form.tpl' ui=$register_form}
        </div>
      </div>
    {/if}
  {/block}

Here is the part of code, what is incorrectly used and do error 500 with PS 8.1
https://github.com/PrestaShop/PrestaShop/blob/e7a1e3e7cd8be3bc7a023b3c03c3bdd51ea4f1dc/themes/classic/templates/checkout/order-confirmation.tpl#L95C10-L95C10

The fix

This PR verifies if proper VALID parameters were submitted with the call, and point the user to faulty template. In production mode, the call will be skipped. In dev mode, it throws a notice.

Follows the same logic as in #31092.
Snímek obrazovky 2023-11-02 124312

@ShaiMagal ShaiMagal requested a review from a team as a code owner November 2, 2023 11:26
@prestonBot prestonBot added 8.1.x Branch Improvement Type: Improvement labels Nov 2, 2023
Hlavtox
Hlavtox previously approved these changes Nov 2, 2023
Copy link
Contributor

@matthieu-rolland matthieu-rolland left a comment

Choose a reason for hiding this comment

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

Seems like the type of $ui should be checked before calling its method (phpstan error)

@M0rgan01 M0rgan01 added this to the 8.1.3 milestone Nov 2, 2023
@Hlavtox Hlavtox force-pushed the smartyrender-condition-for-older-templates branch from 9d7af17 to 153d881 Compare November 7, 2023 13:44
@Hlavtox Hlavtox changed the title Preventing error 500 on order confirmation (guest only) while using outdated theme Validate smarty render calls (prevent errors from outdated themes) Nov 7, 2023
@Hlavtox Hlavtox requested review from PululuK, matthieu-rolland and a team November 7, 2023 17:26
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

You should have incorrect theme
What is "incorrect" 😉 ?

@Hlavtox
Copy link
Contributor

Hlavtox commented Nov 7, 2023

@matks You can simulate this by putting {render file='customer/_partials/customer-form.tpl' ui=$register_form} anywhere into themes\classic\templates\checkout\order-confirmation.tpl ;-)

@ShaiMagal
Copy link
Contributor Author

I saw this problem about 4 times for last month in production PS eshops with theme oficially targeted for PS 8.1, but in reality, with this problem like hlavtox already wrote... :)

@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Nov 10, 2023
@florine2623 florine2623 self-assigned this Nov 14, 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.

Hello @ShaiMagal ,

I do not have the same Notices as you. Only the last one is the same.

Screenshot 2023-11-14 at 11 04 04

Warning: Undefined array key "register_form" in /Users/fHea/Desktop/pr-ps810/var/cache/dev/smarty/compile/classiclayouts_layout_full_width_tpl/c4/d1/e2/c4d1e2f3ba3a5749ec1fa732c5e036efcb8985ce_2.file.order-confirmation.tpl.php on line 98

Warning: Attempt to read property "value" on null in /Users/fHea/Desktop/pr-ps810/var/cache/dev/smarty/compile/classiclayouts_layout_full_width_tpl/c4/d1/e2/c4d1e2f3ba3a5749ec1fa732c5e036efcb8985ce_2.file.order-confirmation.tpl.php on line 98

Could you check those 2 please ? :)

Thanks!

@florine2623 florine2623 removed the Waiting for QA Status: action required, waiting for test feedback label Nov 14, 2023
@florine2623 florine2623 removed their assignment Nov 14, 2023
@florine2623 florine2623 added the Waiting for author Status: action required, waiting for author feedback label Nov 14, 2023
@Hlavtox
Copy link
Contributor

Hlavtox commented Nov 14, 2023

@florine2623 This is the result you should get. :-) It's QA ok. 👍

@florine2623
Copy link
Contributor

Seen with @Hlavtox ,

The warnings are displayed because a bad variable was used on purpose !

It is QA ✅

@florine2623 florine2623 self-assigned this Nov 14, 2023
@florine2623 florine2623 added QA ✔️ Status: check done, code approved and removed Waiting for author Status: action required, waiting for author feedback labels Nov 14, 2023
@Hlavtox Hlavtox merged commit 656c2f7 into PrestaShop:8.1.x Nov 14, 2023
38 checks passed
@Hlavtox
Copy link
Contributor

Hlavtox commented Nov 14, 2023

Good job, thanks everyone!

@ShaiMagal ShaiMagal deleted the smartyrender-condition-for-older-templates branch November 14, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.1.x Branch Improvement Type: Improvement QA ✔️ Status: check done, code approved
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

10 participants