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 category-header.tpl #477

Closed
wants to merge 2 commits into from
Closed

Conversation

rodriciru
Copy link
Contributor

@rodriciru rodriciru commented Mar 27, 2023

close #476

sudo-do
sudo-do previously approved these changes Mar 27, 2023
Copy link
Contributor

@sudo-do sudo-do left a comment

Choose a reason for hiding this comment

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

Since I don't see a reason to get invalid index in line 14.

Copy link
Contributor Author

@rodriciru rodriciru left a comment

Choose a reason for hiding this comment

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

Not necessary IMO

@rodriciru
Copy link
Contributor Author

I dont know how to reject this changes, so i hope i do it ok

@sudo-do
Copy link
Contributor

sudo-do commented Mar 28, 2023

yeah, the change is not nessesary as far as i know prestashop environment

@AureRita AureRita assigned AureRita and unassigned AureRita Apr 3, 2023
@florine2623 florine2623 self-assigned this Apr 3, 2023
Copy link

@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 @rodriciru ,

I tried to install your PR, but I have an error, the theme can't be applied to my FO :

cd themes            
rm -rf hummingbird
git clone https://github.com/PrestaShop/hummingbird.git hummingbird
cd hummingbird
git prc origin 477 dev
cd _dev && npm i && npm run build

Screenshot 2023-04-03 at 14 49 44

I also tested to install your PR on the theme hummingbird v0.1.5.
No PR, hummingbird il well installed :
Screenshot 2023-04-03 at 14 57 33

Once I install your PR with, my FO is broken :
Screenshot 2023-04-03 at 15 00 59

Is there something I'm doing incorrectly ?

Also, could you add a "How to test" with screenshots as well ?

Thanks!

@florine2623 florine2623 removed their assignment Apr 3, 2023
@Hlavtox
Copy link
Contributor

Hlavtox commented Apr 3, 2023

@rodriciru Can you show what error you are getting? I deleted the images from the category but I get no error 🤔

@rodriciru
Copy link
Contributor Author

Hello @rodriciru ,

I tried to install your PR, but I have an error, the theme can't be applied to my FO :

cd themes            
rm -rf hummingbird
git clone https://github.com/PrestaShop/hummingbird.git hummingbird
cd hummingbird
git prc origin 477 dev
cd _dev && npm i && npm run build
Screenshot 2023-04-03 at 14 49 44

I also tested to install your PR on the theme hummingbird v0.1.5. No PR, hummingbird il well installed : Screenshot 2023-04-03 at 14 57 33

Once I install your PR with, my FO is broken : Screenshot 2023-04-03 at 15 00 59

Is there something I'm doing incorrectly ?

Also, could you add a "How to test" with screenshots as well ?

Thanks!

Highly improbable that that was my fault. I think you also delete the CSS when do the rm command. SO you have no CSS so the website look like this. You have to compile the asset's folder, but that has nothing to do with this PR

@rodriciru
Copy link
Contributor Author

@rodriciru Can you show what error you are getting? I deleted the images from the category but I get no error 🤔

More than an error is a PHP warning in server logs. I have fixed that at my store, so I get no more warnings
You are looking for a value that is null, and this throw a warning

@Hlavtox
Copy link
Contributor

Hlavtox commented Apr 3, 2023

@rodriciru It doesn't throw me a warning even if $category.image is null 🤔, the condition catches it without a notice/error. Weird. I checked both PHP and Symfony error logs and nothing in both.

@kpodemski Do you get an error if you have a category without an image and do this? I don't for some reason.

{if !empty($category.image.large.url)}
    {$category.image.large.url}
{/if}

@rodriciru
Copy link
Contributor Author

rodriciru commented Apr 3, 2023

I get a php warning at smarty compile (i suppose if i disable cache i will get something similar)
The warning looks like this, but is not exactly this:
[03-Apr-2023 17:23:21 Europe/Madrid] PHP Warning: Trying to access array offset on value of type null in /usr/home/XXXX/web/var/cache/prod/smarty/compile/XXXXXlayouts_layout_left_column_tpl/00/78/cc/0078ccc9839dcacadc15c0f4aa16f402af5079aa_2.file.category.tpl.php on line 57

imagen

@kpodemski
Copy link
Contributor

@rodriciru that's odd, empty for this key should be safe 🤔 have you checked this file 078ccc9839dcacadc15c0f4aa16f402af5079aa_2.file.category.tpl.php on line 57 to see if it is really about this line?

@rodriciru
Copy link
Contributor Author

rodriciru commented Apr 25, 2023

@rodriciru that's odd, empty for this key should be safe 🤔 have you checked this file 078ccc9839dcacadc15c0f4aa16f402af5079aa_2.file.category.tpl.php on line 57 to see if it is really about this line?

Yep, it's weird as hell, but I have some answers.
Looking at compiled PHP, I see that some empty functions translate to simple if's and others don't.
Concretely, this image empty translates to if and the empty 2 lines above translate to an empty instead of an if:
PLEASE KEEP IN MIND It's NOT EXACTLY THIS FILE, BUT IT'S ALMOST THE SAME
Original smarty tpl:

{if !empty($category.image)}
                    <div class="category-cover">
                        <img src="{$category.image.large.url}" alt="{if !empty($category.image.legend)}{$category.image.legend}{else}{$category.name}{/if}" loading="lazy" width="141" height="180">
                    </div>
                {/if}

Compiled result:

            <?php if ($_smarty_tpl->tpl_vars['category']->value['image']) { ?>
                <div class="category-cover">
                  <img src="<?php echo htmlspecialchars((string) $_smarty_tpl->tpl_vars['category']->value['image']['large']['url'], ENT_QUOTES, 'UTF-8'); ?>
" alt="<?php if (!empty($_smarty_tpl->tpl_vars['category']->value['image']['legend'])) {
                  echo htmlspecialchars((string) $_smarty_tpl->tpl_vars['category']->value['image']['legend'], ENT_QUOTES, 'UTF-8');
                } else {
                  echo htmlspecialchars((string) $_smarty_tpl->tpl_vars['category']->value['name'], ENT_QUOTES, 'UTF-8');
                } ?>">
                </div>
            <?php } ?>

Effectively, empty not raises any warnings, but if does, so this is why we get the warning.
Still, I think this is an easy and no harmful at all fix, even if the blame of the issue is smarty

@ga-devfront ga-devfront reopened this Jul 27, 2023
@leemyongpakvn
Copy link
Contributor

@florine2623 FYI, hummingbird is different from classic-theme. There is no _dev directory in hummingbird, you need to create a .env file inside its webpack directory, and run npm run build from hummingbird directory following this cookbook https://github.com/PrestaShop/hummingbird#how-to-build-assets ;)

@leemyongpakvn
Copy link
Contributor

leemyongpakvn commented Dec 2, 2023

@florine2623 Please check my message above when you have time. By the way, I can't understand the logic of this PR. ;)

@florine2623 florine2623 self-assigned this Dec 5, 2023
@florine2623
Copy link

Hello @rodriciru ,

It seems like no one can reproduce your issue, neither @Hlavtox , @kpodemski nor @leemyongpakvn .

Could you explain further so it can be reproduced and tested ?

@rodriciru
Copy link
Contributor Author

Hard to explain it more.
I think I explain it very well, smarty produces different structures for empty, one throws a WARNING, the other don't.
I still think the PR won't break anything, but if you don't feel right with this, close the issue, as I'm the only who get this.
Thanks!

@leemyongpakvn
Copy link
Contributor

As usual, QA will close the issue if they can not reproduce it 👌

@Hlavtox
Copy link
Contributor

Hlavtox commented Dec 8, 2023

@rodriciru I will try to reproduce once again, if you really say it happens. There is no reason for smarty to convert empty to if. If it does so, it's stupid. 🤔

On what core version you are getting this behavior?

@Prestaworks
Copy link

{if !empty($category.image.large.url)} is also present in classic theme, I tried deleting the category images, "image" => null shows on the category dump, tested on PS 8.1.2 and php 7.2, 7.4 and 8.1 still nothing in the php logs or Prestashop log files.

so if this is caused by "image" not existing it's maybe an older PS version?
To make the check safe it should maybe be something like
{if !empty($category.image) && !empty($category.image.large)}

But I can't recreate the error on PS 8.1.2.

@rodriciru
Copy link
Contributor Author

rodriciru commented Dec 8, 2023 via email

@kpodemski
Copy link
Contributor

@Hlavtox @Prestaworks maybe it was a bug in some Smarty version because I noticed that problem as well, I've had {if !empty($array.key)} it started flooding logs with a warnings/notice.

@Prestaworks
Copy link

@Hlavtox @Prestaworks maybe it was a bug in some Smarty version because I noticed that problem as well, I've had {if !empty($array.key)} it started flooding logs with a warnings/notice.

Do you remember what version of PS it was?

@kpodemski
Copy link
Contributor

kpodemski commented Dec 15, 2023

@Prestaworks 1.7.6 something

@florine2623
Copy link

I suggest closing this issue as it cannot be reproduced #476 (comment)

@florine2623 florine2623 closed this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

If category image don't exist, this will throw a null on php logs
10 participants