Fix all reported bugs #53

MathiasReker · 2018-08-08T11:40:41Z

Fixed all reported bugs.

Tested to work on multistore and multilanguage, with https:// and http://

This change is

eternoendless

Thanks for this awesome contribution! Some minor changes are needed.

eternoendless · 2018-08-08T13:49:49Z

composer.json

-    "preferred-install": "dist"
-  },
-  "type": "prestashop-module"
-}


This file should be kept

eternoendless · 2018-08-08T13:50:57Z

config.xml

+    <displayName><![CDATA[Google sitemap]]></displayName>
+    <version><![CDATA[4.0.0]]></version>
+    <description><![CDATA[Generate your Google sitemap file]]></description>
+    <author><![CDATA[Prestashop]]></author>


A small typo here

-<author><![CDATA[Prestashop]]></author> +<author><![CDATA[PrestaShop]]></author>

eternoendless · 2018-08-08T13:51:08Z

config.xml

 	<limited_countries></limited_countries>
-</module>
+</module>


Please add a trailing LF here

I'm sorry I might be wrong but I cant see the trailing LF 🤔 ?

you are right, fixed here: 724d539

eternoendless · 2018-08-08T13:52:36Z

gsitemap-cron.php

+ * @author    PrestaShop SA <contact@prestashop.com>
+ * @copyright 2007-2018 PrestaShop SA
+ * @license   https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
+ * International Registered Trademark & Property of PrestaShop SA


This is header for Core files (which are licensed using OSL-3.0 license). Please use the modules header (licensed AFL-3.0) instead: https://devdocs.prestashop.com/1.7/development/coding-standards/#module-files

eternoendless · 2018-08-08T13:53:56Z

gsitemap.php

+ * @author    PrestaShop SA <contact@prestashop.com>
+ * @copyright 2007-2018 PrestaShop SA
+ * @license   https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
+ * International Registered Trademark & Property of PrestaShop SA


Same comment about the file header

eternoendless · 2018-08-08T14:34:15Z

views/img/index.php

+ * @author    PrestaShop SA <contact@prestashop.com>
+ * @copyright 2007-2018 PrestaShop SA
+ * @license   https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
+ * International Registered Trademark & Property of PrestaShop SA


Same comment about the header

eternoendless · 2018-08-08T14:34:25Z

views/index.php

+ *
+ * @author    PrestaShop SA <contact@prestashop.com>
+ * @copyright 2007-2018 PrestaShop SA
+ * @license   https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)


Same comment about the header

eternoendless · 2018-08-08T14:34:41Z

views/templates/admin/configuration.tpl

+ * @author    PrestaShop SA <contact@prestashop.com>
+ * @copyright 2007-2018 PrestaShop SA
+ * @license   https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
+ * International Registered Trademark & Property of PrestaShop SA


Same comment about the header

eternoendless · 2018-08-08T14:36:20Z

views/templates/admin/index.php

+ *
+ * @author    PrestaShop SA <contact@prestashop.com>
+ * @copyright 2007-2018 PrestaShop SA
+ * @license   https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)


Same comment about the header

eternoendless · 2018-08-08T14:36:42Z

views/templates/index.php

+ *
+ * @author    PrestaShop SA <contact@prestashop.com>
+ * @copyright 2007-2018 PrestaShop SA
+ * @license   https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)


Same comment about the header

PierreRambaud · 2018-08-08T15:52:14Z

gsitemap.php

+    public function emptySitemap($id_shop = 0)
+    {
+        if (!isset($this->context)) {
+            $this->context = new Context();


Should it be Context::getContext() ?

Both are fine.

Did not change anything.

Should I ? :-)

PierreRambaud · 2018-08-08T15:52:43Z

views/templates/admin/configuration.tpl

+   $(document).ready(function() {
+
+   	if ($('.gsitemap_metas:checked').length == $('.gsitemap_metas').length)
+   		$('.check').parent('label').children('span').html("{l s='uncheck all' d='Modules.Gsitemap.Admin'}");


Can you remove tabs in this file please?

PierreRambaud · 2018-08-08T15:53:02Z

Thanks a lot again! Awesome work!

Quetzacoalt91 · 2018-08-09T07:40:58Z

gsitemap.php

+        $this->name = 'gsitemap';
+        $this->tab = 'seo';
+        $this->version = '4.0.0';
+        $this->author = 'Prestashop';


Following the comment in config.xml, can you please set PrestaShop in author?

Yes, ofc. 👍

Quetzacoalt91 · 2018-08-09T08:35:07Z

@LouiseBonnard can you please have a look at the domains used in this PR?

LouiseBonnard · 2018-08-09T08:52:34Z

@Quetzacoalt91, I can see no domain used here, nor strings to check. Is it?

Quetzacoalt91 · 2018-08-09T08:55:19Z

They are in the file gsitemap.php, for instance: https://github.com/PrestaShop/gsitemap/pull/53/files#diff-3133bb1114a2b4792863d1d6ec46d8d6R51

rGaillard · 2018-08-09T08:57:00Z

gsitemap-cron.php

@@ -31,27 +31,30 @@
 include(dirname(__FILE__).'/../../config/config.inc.php');
 include(dirname(__FILE__).'/../../init.php');
 /* Check to security tocken */
-if (substr(Tools::encrypt('gsitemap/cron'), 0, 10) != Tools::getValue('token') || !Module::isInstalled('gsitemap'))
-	die('Bad token');
+if ('cli' != php_sapi_name()) {


Hello,

In some server configuration, when PHP is running in cgi mode per exemple php_sapi_name() will returns cli. You should use the Tools::isPHPCLI() method.

Is the fix correct?

The http:// will be missing in the sitemap

The use of function header() is forbidden; use Tools::redirect() instead

Fix alert color-success Allow to activate checkbox by clicking on text Fix sitemap links when friendly url is set to off

MathiasReker · 2018-08-09T17:40:06Z

Ping @eternoendless @Quetzacoalt91 @PierreRambaud I made a lot of changes in order to improve the module further. I hope you will consider to merge my changes. :-)

I also fixed bugs from validator:

Outdated

Quetzacoalt91

Code looks good to me

eternoendless

Awesome job @mreker! Only a couple small fixes left!

eternoendless · 2018-08-10T10:53:59Z

gsitemap-cron.php

+include(dirname(__FILE__).'/../../config/config.inc.php');
+include(dirname(__FILE__).'/../../init.php');
+/* Check to security tocken */
+if ('cli' != Tools::isPHPCLI()) {


Tools::isPHPCLI() returns a boolean, so you should change the value your comparing to appropriately.

Either one of these would be fine:

if (!Tools::isPHPCLI()) {

if (false === Tools::isPHPCLI()) {

eternoendless · 2018-08-10T10:56:24Z

gsitemap.php

                'gsitemap_feed_exists' => file_exists($this->normalizeDirectory(_PS_ROOT_DIR_).'index_sitemap.xml'),
                'gsitemap_last_export' => Configuration::get('GSITEMAP_LAST_EXPORT'),
                'gsitemap_frequency' => Configuration::get('GSITEMAP_FREQUENCY'),
-                'gsitemap_store_url' => 'http'.(Configuration::get('PS_SSL_ENABLED') ? 's' : '').'://'.Tools::getShopDomain(false, true).__PS_BASE_URI__,
+                'gsitemap_store_url' => $this->context->link->getBaseLink(),


If you want to take it one step further, you can store $this->context->link->getBaseLink() in a variable to avoid multiple calls.

Like this ? 11265f6

eternoendless · 2018-08-10T11:05:57Z

gsitemap.php

-                    $admin_folder = str_replace(_PS_ROOT_DIR_, '', _PS_ADMIN_DIR_);
-                    $admin_folder = Tools::substr($admin_folder, 1);
-                    header('location: http'.(Configuration::get('PS_SSL_ENABLED') ? 's' : '').'://'.Tools::getShopDomain(false, true).__PS_BASE_URI__.$admin_folder.'/index.php?tab=AdminModules&configure=gsitemap&token='.Tools::getAdminTokenLite('AdminModules').'&tab_module='.$this->tab.'&module_name=gsitemap&continue=1&type='.$new_link['type'].'&lang='.$lang.'&index='.$index.'&id='.(int)$id_obj.'&id_shop='.$this->context->shop->id);
+                    Tools::redirectAdmin($this->context->link->getBaseLink().'index.php?tab=AdminModules&configure=gsitemap&token='.Tools::getAdminTokenLite('AdminModules').'&tab_module='.$this->tab.'&module_name=gsitemap&continue=1&type='.$new_link['type'].'&lang='.$lang.'&index='.$index.'&id='.(int)$id_obj.'&id_shop='.$this->context->shop->id);


Watch out, getBaseLink() will get you a link to your front office instead of the admin. This can be further simplified if you use getAdminLink():

Tools::redirectAdmin( $this->context->link->getAdminLink( 'AdminModules', true, array(), array( 'tab_module' => $this->tab 'module_name' => $this->name, 'continue' => 1, 'type' => $new_link['type'], 'lang' => $lang, 'index' => $index, 'id' => (int)$id_obj, 'id_shop' => $this->context->shop->id, ) )

Sorry, I don't understand. Can you make a commit, then I will check how exactly you solve this?

You can replace the whole line with what @eternoendless sent to you =)

When I did that I get an error. I can't remember the error right now. I can reproduce the error and send a screenshot.

I added a comma after $this->tab. Please see screenshot:

I tried without ; too. What am I doing wrong?

Tools::redirectAdmin( $this->context->link->getAdminLink( 'AdminModules', true, array(), array( 'tab_module' => $this->tab 'module_name' => $this->name, 'continue' => 1, 'type' => $new_link['type'], 'lang' => $lang, 'index' => $index, 'id' => (int)$id_obj, 'id_shop' => $this->context->shop->id, ) ) );

Missing ); :D

Fixed: bf5a4d4

eternoendless · 2018-08-10T11:11:16Z

gsitemap.php

@@ -805,7 +794,7 @@ public function createSitemap($id_shop = 0)
        if ($this->cron) {
            die();
        }
-        header('Location: ./index.php?tab=AdminModules&configure=gsitemap&token='.Tools::getAdminTokenLite('AdminModules').'&tab_module='.$this->tab.'&module_name=gsitemap&validation');
+        Tools::redirectAdmin('index.php?tab=AdminModules&configure=gsitemap&token='.Tools::getAdminTokenLite('AdminModules').'&tab_module='.$this->tab.'&module_name=gsitemap&validation');


I think it's better to use getAdminLink() as explained before.

can you rewrite the code snippets to use getAdminLink(), I am afraid to mess something up. I can't translate the code from the preview example.

PierreRambaud · 2018-08-10T11:55:01Z

@Quetzacoalt91 @eternoendless Need a waiting for QA maybe? Wdyt?

Quetzacoalt91 · 2018-08-10T11:58:12Z

Indeed, but wait for the other review to be completed.

MathiasReker · 2018-08-10T12:06:40Z

I still need to do some work on this @PierreRambaud :-)

…ibility

MathiasReker · 2018-10-16T18:09:52Z

Now there are only one kind of cs issue left: Line exceeds 120 characters. I fixed a lot of those issues, but if I fix the rest, the code will be harder to read.

marionf · 2018-10-17T07:26:52Z

@mreker

I have another exception when I configure the module

MathiasReker · 2018-10-17T07:43:28Z

Sorry! I see the issue. I fix it when I get to my PC in 20 min.

MathiasReker · 2018-10-17T07:55:50Z

@marionf fixed now

marionf · 2018-10-17T09:10:42Z

Thank you @mreker 👍

MathiasReker · 2018-10-17T09:17:24Z

@marionf you can close all other PR's too. :-)

PierreRambaud · 2018-10-17T09:35:54Z

Thanks @mreker (again ;))

MathiasReker · 2018-10-17T09:40:42Z

You are welcome @PierreRambaud :-)

Those can be closed: https://github.com/PrestaShop/gsitemap/issues/30, https://github.com/PrestaShop/gsitemap/issues/24, https://github.com/PrestaShop/gsitemap/issues/16 :-)

MathiasReker · 2018-10-17T12:22:58Z

Changelog:

Bump minimum compatibility to 1.7

Bump PHP require to 5.6.0

Use new translation system (PS 1.7)

Fix issue with generating sitemaps for languages

Fix code standard to meet PSR-2

Fix double generation of CMS link

Fix errors when generating sitemap on multistore

Allow running cron job from command line

Fixed issue with duplicated CMS links in sitemap file on multistore

Remove UTF8 control characters from XML sitemap

Fix issue with meta link generations

Fix errors in Google Search Console

Fix format error: https://www.sitemaps.org/protocol.html

Fix problems with SSL

Fix wrong link generation

Allow generate sitemaps without friendly URLs

Fix multi language issues

Update docs: Fixed links

Update header

Fix button to check fields

Make check boxes clickable from text

Fix check of security token

Add caption on categories

Fix errors and warnings from PrestaShop validator

Fix typos

Fix issues with https:// on images

Remove links to supplier and manufactures

MathiasReker · 2018-11-02T09:59:20Z

Please see this PR: #60

sanctusmob · 2019-11-02T11:13:38Z

Why did you remove manufacturer and suppliers from sitemap?

Fixes

b3e182e

MathiasReker mentioned this pull request Aug 8, 2018

Add gsitemap PrestaShop/PrestaShop#9404

Merged

eternoendless previously requested changes Aug 8, 2018

View reviewed changes

PierreRambaud reviewed Aug 8, 2018

View reviewed changes

Quetzacoalt91 reviewed Aug 9, 2018

View reviewed changes

rGaillard reviewed Aug 9, 2018

View reviewed changes

MathiasReker added 8 commits August 9, 2018 11:52

fixes v2

2f6eb29

fixes3

6b0d207

rollback

c2eb98b

The http:// will be missing in the sitemap

replaced "\r\n" with "\n"

e55f28e

removed new line at end readme

15f91b6

fix bug on validate

51ac27f

The use of function header() is forbidden; use Tools::redirect() instead

rewritten

6df978b

fixes

2121d8c

Fix alert color-success Allow to activate checkbox by clicking on text Fix sitemap links when friendly url is set to off

Quetzacoalt91 approved these changes Aug 10, 2018

View reviewed changes

Fixed link to the docs

75a71e3

eternoendless requested changes Aug 10, 2018

View reviewed changes

MathiasReker added 3 commits August 10, 2018 19:21

fix https on images

b2dfd43

Merge branch 'master' of https://github.com/Mreker/gsitemap-1

3dc7549

Some minor changes

af56312

PierreRambaud removed the waiting for author label Oct 16, 2018

This was referenced Oct 16, 2018

Update domains #41

Closed

Update domains #42

Closed

Update to https on Stores that have https enabled #52

Closed

MathiasReker added 5 commits October 16, 2018 15:43

LF new line

a94706d

use Tools::substr() instead of substr()

c73c56b

Fix: Line exceeds 120 characters

1e165a3

Just some beauty =)

c8e2820

Mothod name should not be prefixed with an underscore to indicate vis…

2f8fd59

…ibility

Fix typos

c48912f

marionf added waiting for author and removed waiting for QA labels Oct 17, 2018

Fix error 500

1ad3047

marionf added QA ✔️ and removed waiting for author labels Oct 17, 2018

PierreRambaud merged commit d9d06d0 into PrestaShop:dev Oct 17, 2018

MathiasReker mentioned this pull request Oct 24, 2018

Fix wrong use of protocol #60

Merged

sanctusmob mentioned this pull request Nov 2, 2019

Re add Manufacturer and Supplier Pages #126

Closed

sanctusmob mentioned this pull request Dec 28, 2021

Re add Manufacturer and Supplier Pages #147

Closed

Fix all reported bugs #53

Fix all reported bugs #53

Conversation

MathiasReker commented Aug 8, 2018 • edited by eternoendless Loading

eternoendless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PierreRambaud commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quetzacoalt91 commented Aug 9, 2018

LouiseBonnard commented Aug 9, 2018

Quetzacoalt91 commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MathiasReker commented Aug 9, 2018

Quetzacoalt91 left a comment

Choose a reason for hiding this comment

eternoendless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PierreRambaud Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PierreRambaud commented Aug 10, 2018

Quetzacoalt91 commented Aug 10, 2018

MathiasReker commented Aug 10, 2018

MathiasReker commented Oct 16, 2018

marionf commented Oct 17, 2018

MathiasReker commented Oct 17, 2018

MathiasReker commented Oct 17, 2018

marionf commented Oct 17, 2018

MathiasReker commented Oct 17, 2018

PierreRambaud commented Oct 17, 2018

MathiasReker commented Oct 17, 2018

MathiasReker commented Oct 17, 2018 • edited Loading

MathiasReker commented Nov 2, 2018

sanctusmob commented Nov 2, 2019

MathiasReker commented Aug 8, 2018 •

edited by eternoendless

Loading

PierreRambaud Aug 13, 2018 •

edited

Loading

MathiasReker commented Oct 17, 2018 •

edited

Loading