Topic bump – another bugfix

Icarus - 15 July 2009 09:54 AM

• There is a problem when output location uses backslashes (Windows) – I did not fix this one, but probably need to use addslashes() somewhere – I also noticed that when running this for the first time (clean configuration), the default path does not contain ANY slash (e.g. D:htdocsMyPathen_googlebase.xml). Of course, workaround is to use forward slashes.
  

Product Link was not properly cleaned on 1.1 – the result from getProductLink comes without domain on 1.1. I fixed this one, but only for 1.1

Added trailing slash for <link> tag

Categories with the sorting prefix (20.Category) were not cleaned for product_type. Fixed.

EDIT:

Another bug fix – categories were not HTML escaped – so categories with & broke the feed. (I used CDATA but maybe you want to use something else).
Updated the attached file (0.6.1c).

I’ll do some testing on a local XP machine and try and work out how to handle the path differences between the two.

Keep ‘em coming

note: I’ll also add the option to select either the long or short product description I think in the next revision as this has been mentioned before (some folks only use the long description field so their feed descriptions are blank).

Paul

Hi Paul,

Great. so:

• Re: product link – I do not have 1.2, but I can tell you it did not work as expected in 1.1 – I will retest your original code, see if I can provide more information for that specific fix. I completely understand the logic you were aiming for and it makes sense.
• Re: windows directories – dont install xampp if you dont want to, I thought you were on Windows. I will play with it and see if I can fix.
• Re: product type escaping – I have categories that contain ampersand, and the XML does not like it. Google asks that we escape > and & or wrap it all with CDATA – so I changed your gt; separator to >, then wrapped everything with CDATA – looked like the easiest solution – published the file to google and it was completely accepted.

For example, this is one line from my output xml

I will report back about #1 and #2 soon.

EDIT:
Re trailing slash – I just have that habbit of always using them, to prevent apache from doing that extra redirect – since I am a heavy user of htaccess rewriterules…

I think the first two items are probably related. It works fine here on my unix boxes, so again I think it may be down to the different filesystems. I’ve got XAMPP set up, so I’ll do a quick 1.1 install and try it. I’ll also test on 1.2 unix – I suspect that this should be sufficient.

The “missing” separators in the directory is a display issue I think, since the “/” is an escape character in the html world. That should be easy to fix – will just have to double them up, then fix them in the config variable again after the form returns.

I understand what you mean about the categories – I’ve changed the getPath() member function to convert to html entities for each segment as they’re added together, which avoids the CDATA (you don’t want to use it unless you have to).

btw send me a PM with your preferred url and i’ll add a credit on my site when the module is next uploaded

Thanks Paul,

I am struggling here with the backslash windows path – since there are so many calls to googlebase_filepath from the config.
I have noticed, in case it helps in your debug, that the Configuration class of presta does not do addslashes before it puts something in the db – so D:\one\two becomes D:onetwo

Also, note that you have a private $_filename that seems to be unused.

So if you already have XAMPP, I will stop my attepmts, you know this code much better than me.

About CDATA or not in product type – the reason I picked the CDATA route is because you are using recursion there, so if you are using some encoding anywhere other than the final return value, you get double escaping. I guess you can easily replace the CDATA (at the current location I placed it) with htmlspecialchars, and still leave the separator unescaped, as I have.

And you are probably right about the product link related to windows path, makes total sense.

I’ve just made the changes to the getPath() function so that as each catagory gets added it first has the numbers removed, then gets wrapped in an html_entities() function call. So it will only be done once for each element in the path. I would like to try and keep that loop as free from clutter as possible so that it doesn’t get completely out of control once more stuff gets added in there. I also think it makes it easier to see what’s going on, by having all the code related to generting the final output in the one place.

I’ll have a look at what to do about the windows paths next

Another revision, although I haven’t fixed the windows file system issue yet I have corrected some logic in the path generation in general as I spotted a major bug while testing on 1.2….

Updates:

1. private member function defaultOutputFile() added to handle getting the default location (bug fix and for future MS-Win compatibility) also fixes bug in original code to generate the default output file/path

2. private member function can_write() modified to allow passing of a filename. and PHP errors suppressed internally to it.

3. private member function getPath() modified to output valid xml

4. Removed 3rd party xml_convert code/licence again – unnecessary compatibility layer

5. Moved POST variable cleaning out of getContent() and into _postValidation() and cleaned up validation logic

6. Added the trailing slash in the <link> tag

7. Uninstall removes configuration variables

Paul

Just a thought for those who have been trying these versions as we go along. As of 0.6.2 clicking uninstall will remove the old configuration values and let you install the module again “fresh”. It is probably a good idea to do an install->uninstall->install loop just to make sure you’re starting from scratch again

Paul

Sounds and looks great.
So only outstanding issue is that Windows users have to use forward slashes in the file field for the time being. Correct me if I am wrong.

For this version, yes that seems to be the only outstanding issue. I’ve tested this version with two unix based sites running 1.1.0.5 and 1.2.0.5(beta 3) and all seems to work fine there. That’s not to say that differences in hosting environments might come into play introducing yet more “undocumented features”

Paul

Paul,

There is still a problem with the product_link on 1.1.
The end result is that it in fact contains both the original PS_BASE_URI and the user configured $psdir.

The reason is this:

So the line following it:

Then, the following line:

Creates an invalid link – it has both $psdir and PS_BASE_URI – as a part of $product_link

So, I think this below code will work for both 1.1 and 1.2

Also, your categories are not htmlescaped properly – I still get ampersands and sort prefix.

I propose to do the sort prefix removal and the html entities only once, at the end of the recursion (at this same line) + replace your gt; separator with the actual > sign – this way it seems to be working as expected.

I am attaching the php modified with the above mentioned changes.

If you dont like the attached solution, then in order to test you should create two categories with ampersand:
One that is a child of home and one that is a child of someone else – this will help in making sure it works in both scenarios (and name them with sort prefixes. )

Hi – What would cause this error:
Notice: Undefined offset: 0 in /var/www/html/shop/modules/googlebase/googlebase.php on line 142

I get it two times when creating a feed file in 6.2. (also got it in the previous version.

Is it caused by characters in my items? Are there some characters you canot use in your descriptions? etc Thanks

@diamond204 : It looks to me like you have products with no image or maybe the “cover” image isn’t set? I would need to look into it more closely. I’ll note down that maybe I should test whether a product has any images, as it would certainly not work as expected in that instance.

@Icarus. Sorry, I’ve realised that my testing methodology isn’t catching these variations properly – I have all my sites installed in the root dir for a start, so I’d made an assumption about the return value from the getProductLink() function that was just plain wrong I’m already testing on 3 different setups, so didn’t bother actually testing them with PS_BASE_URI other than ‘/’ Will fix it now one way or the other.

I will also look at what I’ve done wrong in the category function using the setup you’ve suggested. A thought did strike me that the current method won’t work if someone decides to have a category named, for example “Themes for PrestaShop 1.2”. I’ll maybe try and check whether the sorting option is enabled, and if it is specifically remove the prefix only, rather than doing a replace.

Paul

Sure thing Paul.
Agree there may be issues with topics that contain number-dot-something.
Can probably be solved if the regex is modified to /^\d+\./ and is applied per category and not on the entire path (as you originally intended).
Just keep in mind – when it was only applied at the last line of getPath, it did not work properly – don’t know why.

If you want me to test versions before you release, we can also do that, maybe not to bother everybody with the debugging process…

Right, yet another new version. This one should address:

- correct file paths etc. on windows machines – possibly… – yet another fix for generating the product type (please say I’ve nailed it this time!!!) – yet another fix for the product urls (ditto to above!) – A test for at least one image, else the tag is omitted

Outstanding features/bugfixes:

- just noticed that for 1.2 the price is being output at 6 decimal places instead of two. – need an option to select the long or short product description to be used in the feed.

Paul

EDIT: made a fix in the archive and re-uploaded : line 93 the pipe should, of course be a > now and not the html entity.