aimeos / ai-controller-jobs

Aimeos e-commerce job controllers
https://aimeos.org
GNU Lesser General Public License v3.0
28 stars 17 forks source link

Fix media import with external urls #30

Closed vertexvaar closed 3 years ago

vertexvaar commented 3 years ago

Importing media from CSV which contains a hyperlink results in an exception, because the controller does not check correctly if the file exists in the file system. The force flag on the scale call is used in the media/scale controller to circumvent file system checks, which is required here, too. The regex is the same as the one in \Aimeos\Controller\Common\Media\Standard::getFileContent used to determine external sources.

aimeos commented 3 years ago

Thank you for your PR! Wondering if we can shorten the conditions to:

} elseif( $fs->has( $url ) && ( $refItem->getPreviews() === [] || $refItem->getUrl() !== $url )
    || \Aimeos\MW\Str::starts( $url, ['https:', 'https:', 'data:'] )
) {

Background: The mtime tests are also done in the scale() method and we only support http/https/data URLs for streams without modification times. Can you check if that works too?

vertexvaar commented 3 years ago

Your proposed change throws the exception: Couldn't get file mtime for "/project/app/private/uploads/tx_aimeos/https://cdn.example.com/images/products?id=fe9cb6ee-b8bd-4440-82a5-df31dd97ce51"

You removed the force flag, which will result in the mtime check in \Aimeos\Controller\Common\Media\Standard::scale. We can simplify the condition, but then we have to patch the scale() method in aimeos-core to not check the mtime if the URL has a scheme. https://github.com/aimeos/aimeos-core/blob/2021.04/controller/common/src/Controller/Common/Media/Standard.php#L163

Should be

// Only check mtime if the URL does not start with a scheme
if( !$force && (\Aimeos\MW\Str::starts( $item->getUrl(), ['https:', 'https:', 'data:'] )
    || $is && date( 'Y-m-d H:i:s', $fs->time( $item->getUrl() ) ) < $item->getTimeModified() )
) {
    return $item;
}
aimeos commented 3 years ago

You removed the force flag, which will result in the mtime check in \Aimeos\Controller\Common\Media\Standard::scale. We can simplify the condition, but then we have to patch the scale() method in aimeos-core to not check the mtime if the URL has a scheme.

OK, I see the problem but I would like to avoid checking for URLs in the scale() method. I've changed the patch according to what you said and added special handling for data URLs because they shouldn't be scaled too (usually, they are either very small or SVG images).

Does the current code works correctly now?

aimeos commented 3 years ago

Great and thank you very much for your support!