[LameuleFR]

Questions	Answers
Description?	Stops some module tables from growing for nothing. See below.
Type?	bug fix
BC breaks?	yes, Ps_Facetedsearch::indexAttributes() and the layered_product_attribute table are removed
Deprecations?	no
Fixed ticket?
How to test?	See below.
Sponsor company

• layered_product_attribute hasn't been read since the 3.0 refactor (facets join the combination tables directly), but it was still rebuilt on every product save. On a 100k products catalog it's 790k rows / 45 MB. The upgrade (upgrade/upgrade-5.2.0.php) drops it, and indexAttributes(), which only filled it, is removed.
• Deleting a product left its rows in layered_price_index forever. They're now removed on actionProductDelete. On a shop I checked, more than half of the index was deleted products. The upgrade also purges the rows of products deleted before it.
• invalidateLayeredFilterBlockCache() skips the TRUNCATE when the cache is already empty, so a product import no longer runs one per product.

Third-party code reading layered_product_attribute or calling indexAttributes() breaks: worth a line in the release notes.

How to test

1. Upgrade the module: layered_product_attribute is gone, layered_price_index has no rows left for deleted products, facets still work, including attribute filters.
2. Delete a product: its rows disappear from layered_price_index.
3. "Build attributes and features indexes" and the cron URL still work.

[nicohery]

One thing I noticed after approving: the new hook only covers products deleted from now on. Rows left by products deleted before the upgrade are never purged. Indexing only walks existing products, and "Rebuild entire price index" runs in smart mode (fullPricesIndexProcess(..., true) in the cron controller), so it doesn't recreate the table. On my 9.2.x dev install, 36 of the 71 rows in layered_price_index belong to deleted products.

Since the upgrade script already cleans up, maybe it could purge them too:

Db::getInstance()->execute(
    'DELETE lpi FROM `' . _DB_PREFIX_ . 'layered_price_index` lpi
    LEFT JOIN `' . _DB_PREFIX_ . 'product` p ON p.id_product = lpi.id_product
    WHERE p.id_product IS NULL'
);

I ran it in a rolled-back transaction on MySQL 8.4: it removes exactly those 36 rows.

[nicosomb]

Thanks, the three changes hold up. Review done by reading the module at this head and the core 8.2.x and 9.2.x (the supported range starts at 8.2.0); nothing was run on a shop.

Main points

• The upgrade should also purge the layered_price_index rows of products deleted before it, as nicohery suggested: otherwise the shops this PR targets keep their bloated index.
• The version in the upgrade file name and in @deprecated since 5.1.1 depends on the next release number, which is a maintainer call.
• Removing a product from some shops only (it stays on others) does not fire actionProductDelete, so the rows of the removed shops stay. As far as I can tell that is only growth, not wrong results; it can be a follow-up through actionObjectProductDeleteAfter.
• A third-party module or theme reading layered_product_attribute directly breaks once the table is dropped: worth a line in the release notes.

What works

• layered_product_attribute is not read anywhere any more: the only references left are the DROP in uninstall() and in the upgrade.
• actionProductDelete only fires when the product is fully removed (Product::delete() returns early when hasMultishopEntries() is true, on 8.2.x and 9.2.x), so deleting all the rows of the product is right. The Symfony product deletion also goes through Product::delete().
• The hook is registered at install through getAvailableHooks() and in the upgrade.
• getValue('SELECT 1 ...', false) gets its LIMIT 1 from getRow() and bypasses the SQL cache, so the check is cheap and never stale.

Details inline.

[nicosomb]

+1 to the purge nicohery proposed: indexing only walks existing products and "Rebuild entire price index" runs in smart mode, so nothing else removes the rows of products deleted before the upgrade. Next to the DROP:

Suggested change

	Db::getInstance()->execute('DROP TABLE IF EXISTS `' . _DB_PREFIX_ . 'layered_product_attribute`');
	Db::getInstance()->execute('DROP TABLE IF EXISTS `' . _DB_PREFIX_ . 'layered_product_attribute`');
	Db::getInstance()->execute(
	'DELETE lpi FROM `' . _DB_PREFIX_ . 'layered_price_index` lpi
	LEFT JOIN `' . _DB_PREFIX_ . 'product` p ON p.id_product = lpi.id_product
	WHERE p.id_product IS NULL'
	);

[nicosomb]

Question for the maintainers rather than the author: the module is at 5.1.0 and this file only runs when the next release is 5.1.1 or later. As the PR drops a table and deprecates a public method, is the next release a patch or 5.2.0? In the second case the file name, the function name and the @deprecated since 5.1.1 in ps_facetedsearch.php need the new number.

[nicosomb]

Third-party code calling indexAttributes() now gets true silently. A deprecation notice would tell them, as the core does in its deprecated methods:

Suggested change

	public function indexAttributes($idProduct = null)
	{
	if (null === $idProduct) {
	$this->getDatabase()->execute('TRUNCATE ' . _DB_PREFIX_ . 'layered_product_attribute');
	} else {
	$this->getDatabase()->execute(
	'DELETE FROM ' . _DB_PREFIX_ . 'layered_product_attribute
	WHERE id_product = ' . (int) $idProduct
	);
	}
	return $this->getDatabase()->execute(
	'INSERT INTO `' . _DB_PREFIX_ . 'layered_product_attribute` (`id_attribute`, `id_product`, `id_attribute_group`, `id_shop`)
	SELECT pac.id_attribute, pa.id_product, ag.id_attribute_group, product_attribute_shop.`id_shop`
	FROM ' . _DB_PREFIX_ . 'product_attribute pa' .
	Shop::addSqlAssociation('product_attribute', 'pa') . '
	INNER JOIN ' . _DB_PREFIX_ . 'product_attribute_combination pac ON pac.id_product_attribute = pa.id_product_attribute
	INNER JOIN ' . _DB_PREFIX_ . 'attribute a ON (a.id_attribute = pac.id_attribute)
	INNER JOIN ' . _DB_PREFIX_ . 'attribute_group ag ON ag.id_attribute_group = a.id_attribute_group
	' . ($idProduct === null ? '' : 'AND pa.id_product = ' . (int) $idProduct) . '
	GROUP BY a.id_attribute, pa.id_product , product_attribute_shop.`id_shop`'
	);
	return true;
	}
	public function indexAttributes($idProduct = null)
	{
	@trigger_error(
	'Ps_Facetedsearch::indexAttributes() is deprecated since 5.1.1 and does nothing.',
	E_USER_DEPRECATED
	);
	return true;
	}

[LameuleFR]

@nicohery @nicosomb Applied both suggestions in 6895651: the upgrade purges the price index rows of products deleted before it, and indexAttributes() raises a deprecation notice.

For the partial shop removal: a hook alone wasn't enough, indexProductPrices() wrote rows for every shop without checking product_shop, and wrote them back on each save. Fixed in #1364, stacked on this PR. It also widens the upgrade purge to join product_shop instead of product, so it removes the rows of shops a product isn't in as well as those of deleted products. Both PRs should ship in the same release.

[Hlavtox]

@LameuleFR Hi, I would just bump it to 5.2, drop the tables and go on. We don't really need to promise any backward compatibility of modules.

[LameuleFR]

@Hlavtox Done: upgrade script moved to upgrade-5.2.0.php, indexAttributes() removed outright instead of deprecated. The cron action=indexAttributes URL keeps its name since existing crons call it; it still indexes features and attribute groups. Version bump left to your release PR.

This is the final change. Let's merge this fix !

[Hlavtox]

@LameuleFR Hi Benjamin, last change, bump the version in module file and config.xml, then it's ready to go. :-)

[LameuleFR]

Done, version bumped to 5.2.0 in both files. Thanks!