Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow to provide a filename for assets created from URL #1336

Merged
merged 5 commits into from
Nov 25, 2023

Conversation

i-just
Copy link
Contributor

@i-just i-just commented Jun 28, 2023

Description

Adds the ability to provide a filename for importing assets via the assets field if you choose to "Create asset from URL".

New options.filenameNode dropdown field is added below the conflict selection, where user can map the filename they want to use (see bottom of the screenshot).

Screenshot 2023-06-28 at 15 46 38

The mapped filename will be used when uploading images via absolute URL to check if the file already exists. If the filename is not provided via options.filenameNode mapping, then one will be parsed from the URL, as it’s been happening so far.

The conflict resolution method is respected in the same way regardless of whether the separate filename is provided or not.

Example feed:

{
  "entries": [
    {
      "title": "test1",
      "asset": {
        "url": [
          "https://steamcdn-a.akamaihd.net/steam/apps/381780/capsule_616x353.jpg",
          "https://steamcdn-a.akamaihd.net/steam/apps/970830/capsule_616x353.jpg"
        ],
        "name": [
          "img1",
          "img2"
        ]
      }
    },
    {
      "title": "test2",
      "asset": {
        "url": "https://steamcdn-a.akamaihd.net/steam/apps/970830/capsule_616x353.jpg",
        "name": "img2"
      }
    },
    {
      "title": "test3",
      "asset": {
        "url": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAJYAAACWBAMAAADOL2zRAAAAG1BMVEXMzMyWlpaqqqq3t7fFxcW+vr6xsbGjo6OcnJyLKnDGAAAACXBIWXMAAA7EAAAOxAGVKw4bAAABAElEQVRoge3SMW+DMBiE4YsxJqMJtHOTITPeOsLQnaodGImEUMZEkZhRUqn92f0MaTubtfeMh/QGHANEREREREREREREtIJJ0xbH299kp8l8FaGtLdTQ19HjofxZlJ0m1+eBKZcikd9PWtXC5DoDotRO04B9YOvFIXmXLy2jEbiqE6Df7DTleA5socLqvEFVxtJyrpZFWz/pHM2CVte0lS8g2eDe6prOyqPglhzROL+Xye4tmT4WvRcQ2/m81p+/rdguOi8Hc5L/8Qk4vhZzy08DduGt9eVQyP2qoTM1zi0/uf4hvBWf5c77e69Gf798y08L7j0RERERERERERH9P99ZpSVRivB/rgAAAABJRU5ErkJggg=="
      }
    },
    {
      "title": "test4",
      "asset": {
        "url": "https://steamcdn-a.akamaihd.net/steam/apps/278360/capsule_616x353.jpg-|||-https://steamcdn-a.akamaihd.net/steam/apps/970830/capsule_616x353.jpg",
        "name": "img4-|||-img2"
      }
    }
  ]
}

Related issues

#749

@i-just i-just marked this pull request as ready for review June 28, 2023 15:05
@i-just i-just requested a review from angrybrad as a code owner June 28, 2023 15:05
@angrybrad
Copy link
Member

@i-just interesting - I suppose this is a good solution if you have control over the formatting of the feed.

What about the case where it’s a 3rd party feed (which is what sounds like the original issue is), and you only have this?

"asset": {
        "url": [
          "https://steamcdn-a.akamaihd.net/steam/apps/381780/capsule_616x353.jpg",
          "https://steamcdn-a.akamaihd.net/steam/apps/970830/capsule_616x353.jpg"
        ],
}

I’m guessing this PR doesn’t address that (but correct me if I’m wrong).

If that’s the case, I’m guessing we recommend using an event (like Assets::EVENT_STEP_BEFORE_PARSE_CONTENT so people can dynamically rename the files at runtime?

Because the full URIs for an asset in a feed should be unique, and if we wanted to make this automagical, maybe we create a hash out of the URI for the imported filename?

@i-just
Copy link
Contributor Author

i-just commented Jul 4, 2023

What about the case where it’s a 3rd party feed (which is what sounds like the original issue is), and you only have this?
I’m guessing this PR doesn’t address that (but correct me if I’m wrong).

You are 100% correct. I forgot about the case where you can’t control the feed. Let me give it a bit more thought!

@i-just i-just marked this pull request as draft July 4, 2023 07:57
@i-just
Copy link
Contributor Author

i-just commented Jul 5, 2023

Assets::EVENT_STEP_BEFORE_PARSE_CONTENT

Do you mean craft\feedme\services\Process::EVENT_STEP_BEFORE_PARSE_CONTENT?
That could be used if you don’t control the feed, but achieving this would be pretty complex (unless I’m missing a trick here?).

You’d have to go through the field mapping, get the desired/all Assets fields; then hook up “fake” filenameNode mapping, then go through the feed data and for items that match the desired/all Assets fields, add the feed data that matches the “fake” mapping; when matching the mapped Assets field node to the data feed items, you’d have to remember that the node might match exactly, or the value could have been an array, that it can be delimited etc.

Something along those lines could work for JSON:

Event::on(
    Process::class,
    Process::EVENT_STEP_BEFORE_PARSE_CONTENT,
    function(FeedProcessEvent $event) {

        $assetFields = [];
        
        // get handles and nodes for all the asset fields
        foreach ($event->feed['fieldMapping'] as $handle => $fieldInfo) {
            if (isset($fieldInfo['field']) && $fieldInfo['field'] === \craft\fields\Assets::class) {
                $assetFields[$handle] = $fieldInfo['node'];
            }
        }

		// if we have any asset fields
        if (!empty($assetFields)) {
            foreach ($assetFields as $handle => $node) {
            	// for each one, map the filenameNode to a "fake" node name
                $event->feed['fieldMapping'][$handle]['options']['filenameNode'] = 'asset/eventName';

                // go through the feed data
                foreach ($event->feedData as $key => $value) {
                    $regex = str_replace('/', '\/', $node) . '(\/\d+)?';
                    // if the node matches the key we're on, keeping in mind that 
                    // if the $node is 'assets/url' the key could be just 'assets/url' 
                    // or, if an array was provided, it could be 'assets/url/0', 'assets/url/1' etc
                    if (preg_match('/' . $regex . '/', $key, $matches)) {
                        if (isset($matches[1])) {
                            $event->feedData['asset/eventName' . $matches[1]] = md5($value);
                        }
                    }
                    // ... further code for other cases; 
                    // e.g. preg_match is found but not with a number at the end
                    // if the value is delimited etc

                }
            }
        }
    }
);

I looked at what we have available, and couldn’t find an event that would make this easier, so I opted for adding a new one, just for the Assets field for specifying filenames. It’s one of two relation fields where you can’t choose what to use to match with existing data, so I think it makes sense (the other one is the Tags field, a match is done based on the title there). It drastically simplifies the developer’s effort (compared to the above snippet) and allows you to choose how you want to name the files; You can go with md5, with what OP originally suggested etc.

So, if you can control the feed - you can provide and map the filenames. If you can’t, you can do it via the new event.

Event::on(
    Assets::class,
    Assets::EVENT_ASSET_FILENAME,
    function(AssetFilenameEvent $event) {
        $values = $event->fieldValue;
        $filenames = $event->filenames;

        foreach ($values as $key => $value) {
            if (UrlHelper::isAbsoluteUrl($value) && !isset($filenames[$key])) {
                $filenames[$key] = md5($value);
            }
        }

        $event->filenames = $filenames;
    }
);

LMK your thoughts when you have a sec.

@i-just i-just marked this pull request as ready for review July 5, 2023 10:40
@angrybrad
Copy link
Member

@i-just

So, if you can control the feed - you can provide and map the filenames. If you can’t, you can do it via the new event.

Yup, I think this is great - covers both bases and is much cleaner than what I suggested.

@juddlyon
Copy link

juddlyon commented Sep 6, 2023

This looks like a great solution, is there an ETA? This issue happens when importing YouTube thumbnails from their official RSS feeds, e.g. https://i2.ytimg.com/vi/xyz123/hqdefault.jpg

@binarymonkey84
Copy link

Is there any chance this can be reviewed and merged in? The bug this PR addresses is causing a major headache for us trying to migrate a publisher with 20k+ posts coming from WordPress, and a massive chunk of them are not getting the correct image files imported. Too many to manually fix!

# Conflicts:
#	src/web/assets/feedme/dist/FeedMe.js
#	src/web/assets/feedme/dist/FeedMe.js.map
…ets via the assets field if you choose to “Create asset from URL”.
@angrybrad angrybrad merged commit d134755 into v4 Nov 25, 2023
3 checks passed
@angrybrad angrybrad deleted the bugfix/749-asset-filename-conflicts branch November 25, 2023 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants