Image location is a mess

[musicpanda]

I installed a fresh TB 1.4 and ran the module upgrade. The resulting home page on the front side shows at the top a big slider with the "image missing" camera.

Analyzing it I found that it looked for images at /modules/homeslider/images/sample-5.jpg. (with also 4 and 6).

Looking at the file system I found that there doesn't exist an "images" subdirectory under the homeslider module. However, there is a fixtures directory that contains images (1-3 instead of 4-6).

So I went in the backoffice to configure the Homeslider module. On that page too I found missing images. When I checked it looked for images in the /img/homeslider directory. That directory existed but was empty.

[getdatakick]

It's not a mess in the module, it's a mess in the theme template override.

Theme homeslider template makes assumption about image location, and points directly to /modules/homeslider/.

Module now stores images inside /img/ directory instead.

Fix your theme

[musicpanda]

This is a fresh TB installation that still has its original Niara theme. It doesn't have any overrides.

[musicpanda]

I tried to uninstall and then re-install the module in the hope that that might fix the settings. It created new strange effects.

The interesting table is tb_homeslider_slides_lang. Prior to the reinstall it showed images that didn't have matching legends. For example the image with the legend "sample-1" had filename sample-4.jpg. After the reinstall the sample numbers became matching but the filenames got strange prefixes.

[musicpanda]

Ok, I understand that the template in the /themes/niara/modules/homeslider is the problem. But why would anyone want to change directories? Don't fix it if it isn't broken.

Now you have a product that doesn't work out of the box. That is a shame.

[getdatakick]

Product works out of the box. It stopped working after you updated the module to newest version. The homeslider module version that comes with installation package works fine.

We had to fix the module, because every time module was updated, all image files were deleted -- because they were stored inside /modules/homeslider directory that was deleted during update.

I consider this broken. Hence the fix.

We have did everything possible to mitigate this 'directory move' issue. The module comes with updated template. The templates in both niara and community theme module were updated as well. The module contains update functionality to move existing images to new location...

Unfortunately, we can't really force people to update themes before they update modules.

This is general problem with themes. Every time theme developer decides to override some module template it risks future compatibility. Ideally, theme should tweak modules functionality by css only. But sometimes css is not enough, and template overrides are needed.

[musicpanda]

"every time module was updated, all image files were deleted"

Who does that? That is not normal. The working of updating used to be that the files of the zip file were copied over the existing installation of the module. All modules on the market trust that it works that way. If that has somehow been changed to deleting all files that it is disastrous decision that should be turned back as soon as possible. It undermines PS-compatibility and it ignores that not only images are stored by modules but also all kinds of other configuration data.

Storing module images under the /img directory will result in more garbage files as it unlikely that the files will be deleted when the module is deleted.

[getdatakick]

Who does that? That is not normal.

Yes, we do that, and we have a lot of very good reasons for that.

The most important one is the autoloading functionality. A lot of modules using some sort of autoloader to automatically load every php file inside some directory (classes, src,...). If we simply extracted zip file into existing module directory, this autoloader could load old files that are no longer part of new module version.

Similarly problem is with template overrides, javascript and css files that can be automatically added to page source, etc.

There are other reasons behind this. For example, /modules/ directory is (by default) blocked for crowlers by robots.txt file. Which means that any asset (images, generated csv/xml files) were not accessible by robots (google merchant center indexer, for example)

Ideally, content of /modules/ directory should 100% match the content of zip file distribution package.

[musicpanda]

For a product like Thirty Bees with a tiny marketshare having compatible modules and themes is crucial. This change makes a large part of them incompatible with Prestashop 1.6.1. Sure, when you know the way you can fix them. But few people will do that. Many more will be unpleasantly surpised when they discover their data gone.

Also there is no chance that module and theme makers will adapt their software for this "feature". Users installing them will conclude incompatibility. And as they are already facing trouble as many modules and themes need adaptations to run under PHP 8 they will conclude that the trouble is just too much.

The priciple of Prestashop has always been that each module is responsible for its own directory. It is also their job to make sure that when they upgrade this is done correctly. It is their job that when they do autoloading it functions correctly.

The correct way to handle this kind of innovation is to use a flag. When a module wants to have its directory erased it should contain a special flag and only then this procedure should be done.

Deleting directories for some autoload feature reminds of the adoption of Symfony by Prestashop. In both cases infatuation with new technology causes decisions that make a clean break with the past. But Prestashop had at least the decency to enforce that only adapted modules would be used with the new setup. Also, it had the market share that made such a move viable.

That brings me to my last point: you are deliberately wiping out data and that way you are harming the shop owner. This is not a small thing. It is big. It will make TB liable for damage claims. Software bugs happen all the time, but this is deliberate. In most cases the damage will be too small to consider a court case, but when it came to one TB would certainly lose. You can add to that that Thirty Bees will lose all credibility that it has when someone starts making noise because he lost valuable data. If TB is happy to delete your slide shows and your menu's, how can you trust it with your other webshop data?

[getdatakick]

This cleaning directory behaviour is applied only for modules installed through thirty bees api = native modules only. We want our modules and update process to be reliable and consistent, even for modules that use "new technology" like autoloading. There were few native modules that were problemantic, like homeslider etc, hence this kind of changes.

For third party modules, where update is performed by manual zip upload, there is a merge approach. So no compatibility changes. Yet. We want to implement hybrid module update -- delete all code-related files (php/tpl/js/css), but keep assets like images, xml or csv files, etc.

[musicpanda]

I am glad to read that the damage is limited. But some of my points remain:

• this "update" deletes customer data. That is a violation of one of the core software engineering principles.
• this "update" makes existing templates less compatible.
• due to this "update" downloading TB 1.4 and updating it to the latest module version (as most users will do) results in a partially broken shop.
• I don't like the /img/homeslider path. This should be /img/modules/homeslider

The obvious solution would be what Prestashop did when it released 1.7: it renamed the modules. For example, in 1.7 homeslider has been renamed to ps_imageslider. That way changes can be introduced without overwriting customer data or running into problems with themes.

[musicpanda]

"We want to implement hybrid module update -- delete all code-related files (php/tpl/js/css), but keep assets like images, xml or csv files,"

It is tricky to assume that program files don't contain configuration data. Prestashop itself sets the example with "settings.inc.php" and "parameters.php". I have seen js files with a similar function too.

I see this as "too smart" solutions. On good days they save a marginal amount of work. On bad days people spend days to find the cause of a problem because they don't expect this kind of functioning.

It is better to stick to the KISS principle: keep it simple.

[musicpanda]

Further research. It isn't the country field.

Sometimes it works. Sometimes it doesn't.

Note that the country is stored as a session variable until the database is created.