akeeba / fof

Rapid Application Development framework for Joomla!™ 3 and 4
0 stars 0 forks source link

Eager loader on relationship is broken when there are duplicated values #681

Closed tampe125 closed 5 years ago

tampe125 commented 5 years ago

Just discovered that eager loading is broken when the table has some repeated values in the foreign model.
The problem is caused by some weird mechanism inside Joomla Registry object, which triggers a bug in the framework.

Scenario

Company table linked with towns. Towns table has something like 8k records, so if eager loading is not properly enforced you'll discover it quickly. Relation is defined in the company profile as follow:

$this->belongsTo('town', 'Towns', 'foobar_town_id', 'foobar_town_id');
$this->addBehaviour('Filters');
$this->with(['town']); // Eager loading always on

Then you visit the browse page. Bear with me since it will be a terrible journey.
Standard relation logic works as usual, then in \FOF30\Model\DataModel\Relation\HasMany::filterForeignModel() FOF looks at the actual values so he will filter the foreign model. Then later he removes duplicates:

$values = array_unique($values);

Remember this, since it will bother us later Then the foreign model is loaded, filters kicks in. At /Model/DataModel/Behaviour/Filters.php:87 Joomla Registry object is created, passing the values. That object, in the bindData method, will try to understand if the array we passed is associative or not. And how does it? It iterates over the key values and their index. Flashback to array_unique(): duplicated will be removed, preserving the indexes. So we have something like

array (
  0 => '47011',
  1 => '0',
  2 => '100005',
  3 => '100003',
  6 => '47021',
  7 => '47014',
  11 => '48017',
  13 => '47002',
  14 => '47010',
  17 => '53006',
)

The ArrayHelper::isAssociative check will return true, so Joomla Registry will convert our array into a standard class. Second piece of the bug.

Finally, \FOF30\Model\DataModel\Filter\AbstractFilter::exact() will check is the value we passed is an array, but since Joomla converted it into a stdClass the check will fail and the whole 8k rows will be loaded.

We could tackle this bug in several ways, since there's a back and forth on values. Please think about it while I'm going to pour a gallon of alcohol.

nikosdion commented 5 years ago

If I understand correctly, the linchpin for this issue is that array keys are preserved when we filter out unique values. We could just do $values = array_values(array_unique($values)); which re-keys the array. This appears to have minimal impact overall, preserving backwards compatibility.

Can you please try that with your existing use case? I wouldn't want to have to write a component to reproduce the issue when you already have a perfectly reproducible case :)

tampe125 commented 5 years ago

Yep, that does the trick. Honestly that's a terrible workaround, since one could argue that Joomla Register should return the same value we put inside it. The only solution would be to completely bypass it, but hey...

nikosdion commented 5 years ago

There's a legit reason why Registry works like that.

What I proposed should have minimal impact, so it's a good solution actually.

Can you please make two PRs, one to FOF 3 development and one to FOF 4 branch to implement it?