Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Fix Dbafs::addResource and native record order. #7440

Closed
wants to merge 1 commit into from
Closed

Fix Dbafs::addResource and native record order. #7440

wants to merge 1 commit into from

Conversation

tristanlins
Copy link
Contributor

If the native record order does not match the paths order, the Dbafs::addResource return null.

Short example:

$arrPaths = array(
    `files/Media`,
    `files/Media/Images`,
    `files/Media/Images/Bild.jpg`
);

In the database, the records in native order:

ID PID Path isDir
1 3 files/Media/Images/Bild.jpg
2 0 files/Media X
3 2 files/Media/Images X

This situation may happen, when you first create Bild.jpg in another directory, and then move it into a directory that is created later.

Then \FilesModel::findMultipleByPaths($arrPaths) will return:

Collection(
  Model(files/Media/Images/Bild.jpg),
  Model(files/Media),
  Model(files/Media/Images)
)

But the following logic will only work if the last item in the collection is the final file model.


Another solution may be to move the if condition into the while loop, like this:

if ($objModels !== null)
{
    while ($objModels->next())
    {
        if (($i = array_search($objModels->path, $arrPaths)) !== false)
        {
            unset($arrPaths[$i]);
            $arrPids[$objModels->path] = $objModels->uuid;
        }

        // Store the model if it exists
        if ($objModels->path == $strResource)
        {
            $objModel = $objModels->current();
        }
    }
}

@leofeyer leofeyer added this to the 3.2.16 milestone Nov 17, 2014
@leofeyer
Copy link
Member

AFAIU, array('order' => 'path') orders the result by the path length. But is it always true that the file path is the longest? Should we rather order by isDir or even by both path and isDir?

@tristanlins
Copy link
Contributor Author

But is it always true that the file path is the longest?

They must sort in an order, that the parent directories comes first. And if I'm not wrong, the parent paths are always shorter than their children pathes ;-)

@leofeyer
Copy link
Member

What if there are multiple paths in $arrPaths?

$arrPaths = array(
    'files/Media',
    'files/Media/Images',
    'files/Media/Images/Bild.jpg',
    'files/Audio/RidiculouslyLongDirectoryName'
);

I didn't test it yet, so this is a theoretical question. Your second idea to move the if condition inside the while loop seems to be the better choice anyways.

@tristanlins
Copy link
Contributor Author

How could that be in the Dbafs::addRessource method?
Look into the code that generate the $arrPaths https://github.com/tristanlins/contao-core/blob/patch-3/system/modules/core/library/Contao/Dbafs.php#L63,L75

I think your example is absolutely theoretically, but if this would be possible, my fix won't work properly, right.
But in practice, this wont happen with the current algorithm ;-)

@leofeyer
Copy link
Member

That's why I said "I didn't test it yet, so this is a theoretical question" :)

So, which one of the fixes do you prefer? Order by path or move the if condition inside the while loop?

@discordier
Copy link
Contributor

I'd vote for moving the condition into the while loop.

I guess it will be faster to compare all resulting models in PHP than letting MySQL first filter all of them by path (causing a string compare on each of them) and than letting MySQL finally sort them by the exact same string (causing yet another string compare on them).
As we do not have an index available here I guess it will cost us more to lever this on MySQL, especially when dealing with lots of files.

However, I also have not put any test on this to proof my guess.

@tristanlins
Copy link
Contributor Author

👍

@leofeyer
Copy link
Member

Fixed in b11d911 then.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants