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

Re added manufacturers list #173

Merged
merged 6 commits into from Jan 5, 2024
Merged

Re added manufacturers list #173

merged 6 commits into from Jan 5, 2024

Conversation

Uhor
Copy link
Contributor

@Uhor Uhor commented Dec 30, 2023

Questions Answers
Description? Re add Manufacturer (list) to Sitemap XML. Was in the version 3.2.2 but disappeared. I personally need it and I found others that need it too (like in this issue#17771). Someone tried to get it back, but the PR was closed #126. We also remove the calls to tableColumnExists (**) Keep the method in case is need in the future.
Type? "new feature" (was removed with no reason as far as I know)
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#17771.
How to test? Create a Manufacturer (Try adding one with image another without to check the image link) and regenerate Sitemap. You will see the URL of that manufacturer and some extra info.

(**) by Hlavtox

We can remove all calls to tableColumnExists along with that method. The Db structure is the same since 1.7.1.0)

Improvements for future versions:
It will be great to have a "checkbox" to let the user easily check if wants it or not in the Sitemap.
Also, have the option to choose which one you want in your sitemap (because maybe you just want the 3 best.

A screenshot of the test I made:

Sitemap_result

@Uhor Uhor changed the title Added manufacturers list Re added manufacturers list Dec 30, 2023
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.

Few comments, I suggest checking the latest code from getCategoryLink method for inspiration. ;-)

gsitemap.php Outdated Show resolved Hide resolved
gsitemap.php Outdated Show resolved Hide resolved
gsitemap.php Outdated Show resolved Hide resolved
gsitemap.php Outdated Show resolved Hide resolved
gsitemap.php Outdated Show resolved Hide resolved
@Uhor
Copy link
Contributor Author

Uhor commented Jan 2, 2024

Should I change the version to 4.4.0 and add an upgrade file with this?

Configuration::updateValue('GSITEMAP_PRIORITY_MANUFACTURER', 0.7);

boherm
boherm previously approved these changes Jan 3, 2024
@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 3, 2024

@Uhor Adrian, some last tweaks, can you check the tests?

CS fixer is complaining aboout some lines, no space after casting etc.

PHPStan is complaining about this, $image_link will always be set.

$manifacturer_image = [];
            if (isset($image_link)) {
                $manifacturer_image = [
                    'title_img' => htmlspecialchars(strip_tags($manufacturer->name)),
                    'caption' => htmlspecialchars(strip_tags($manufacturer->short_description)),
                    'link' => $image_link,
                ];
            }

Change it to

                $manifacturer_image = [
                    'title_img' => htmlspecialchars(strip_tags($manufacturer->name)),
                    'caption' => htmlspecialchars(strip_tags($manufacturer->short_description)),
                    'link' => $image_link,
                ];

You can also fix the typo - manifacturer_image to manufacturer_image

@Hlavtox Hlavtox added this to the 4.4.0 milestone Jan 3, 2024
@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 3, 2024

@Uhor OK, PHPStan is green. 🎉 Now:

  1. Please add the 4.4.0 upgrade file and bump the version in both module file and config.xml.
  2. Fix the complains of CS fixer https://github.com/PrestaShop/gsitemap/actions/runs/7396696751/job/20122637824?pr=173
  3. I think we could use the built in method to get manufacturer image... Could you try changing this?
$image_link = 'http'.(Configuration::get('PS_SSL_ENABLED') && Configuration::get('PS_SSL_ENABLED_EVERYWHERE') ? 's' : '').'://'.Tools::getMediaServer(_THEME_MANU_DIR_)._THEME_MANU_DIR_.((file_exists(_PS_MANU_IMG_DIR_.'/'.(int)$manufacturer->id.'-medium_default.jpg')) ? (int)$manufacturer->id.'-medium_default' : $lang['iso_code'].'-default-medium_default').'.jpg';

to

$image_link = $this->context->link->getManufacturerImageLink((int) $manufacturer->id, ImageType::getFormattedName('medium'));

And I think we will be good to go. :-)

@Uhor
Copy link
Contributor Author

Uhor commented Jan 3, 2024

Changes applied. Should be fine now.

@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 3, 2024

@Uhor OK, good. Now PHPStan is complaining because there was a bad PHPDoc for the method we used - https://github.com/PrestaShop/PrestaShop/blob/aab3f53c9c31d79a6c60d9744d2a175c4d6c5194/classes/Link.php#L1038

It was fixed in PrestaShop 1.7.8.

So, could you please go to /tests/phpstan and add
- '#Parameter \#2 \$type of method LinkCore\:\:getManufacturerImageLink\(\) expects null, string given.#'
to phpstan-1.7.1.2.neon > phpstan-1.7.7.neon?

@Uhor
Copy link
Contributor Author

Uhor commented Jan 3, 2024

@Hlavtox Done! :)

@Uhor
Copy link
Contributor Author

Uhor commented Jan 4, 2024

After checking the error, I'm wondering if meta_description of manufacturer is better than short_description, as in the product method (strip_tags($product->meta_description). Not because the type, just because the "field meaning".

Also for category maybe, now:
Tools::substr(htmlspecialchars(strip_tags($category->description)), 0, 350)

@Uhor
Copy link
Contributor Author

Uhor commented Jan 4, 2024

After some searching, Google has stopped using the tags <image:caption>, <image:geo_location>, <image:title>, <image:license>, so we can also delete that part from the code.

links:

@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 4, 2024

@Uhor I don't understand why it's reporting that $manufacturer->short_description is an array when you are constructing it with a specific language. 🤔 Just add it to the list of ignored errors for 1.7.8 please. :-)

After some searching, Google has stopped using the tags image:caption, image:geo_location, image:title, image:license, so we can also delete that part from the code.

Hmmm, I didn't know! :-) Yes, we can remove it, but I would do it in a separate PR, so we don't mix and match subjects.

@Uhor
Copy link
Contributor Author

Uhor commented Jan 4, 2024

@Uhor I don't understand why it's reporting that $manufacturer->short_description is an array when you are constructing it with a specific language. 🤔 Just add it to the list of ignored errors for 1.7.8 please. :-)

In the Manufacturer.php it only says array<string> for $description and $short_description
In category, it says @var mixed string or array of Description
In product @var string|array
So maybe is because the doc?

After some searching, Google has stopped using the tags image:caption, image:geo_location, image:title, image:license, so we can also delete that part from the code.

Hmmm, I didn't know! :-) Yes, we can remove it, but I would do it in a separate PR, so we don't mix and match subjects.

Ok ok, although Google still accept the syntax. But I probably will! (less code in file)

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

Thanks for the PR. LGTM.

Brands are well generate with the sitemap now :
Screenshot 2024-01-05 at 10 47 35

@Hlavtox Hlavtox merged commit a25fce1 into PrestaShop:dev Jan 5, 2024
13 checks passed
@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 5, 2024

Thank you @florine2623 & @Uhor!!!

@Uhor
Copy link
Contributor Author

Uhor commented Jan 5, 2024

Thanks to you all! :D

@Uhor Uhor deleted the add-manu-list branch January 19, 2024 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
6 participants