Add asset reference updating. #149

stuartcusackie · 2022-07-28T19:52:18Z

I have tested this manually, and I think my test file is 99% ready. Unfortunately I have no idea how to run this test locally!!

I'm running this from my project folder:
php artisan test vendor/spatie/statamic-responsive-images

But I'm getting a Uncaught Error: Class "Spatie\ResponsiveImages\Tests\TestCase" not found

ncla · 2022-07-28T20:07:27Z

I have such setup going for this addons development: plain Statamic install, where this addon is installed through symlink in project directory. In projects composer.json it looks like this.

"require": {
        "spatie/statamic-responsive-images": "dev-main",
},
"repositories": [ 
        {
            "type": "path",
            "url": "./statamic-responsive-images"
        }
],

On top of that I have Laravel Sail for the project, but I do not think you need it. Then I SSH into the Docker instance, navigate to ./statamic-responsive-images and then from there I simply run ./vendor/bin/phpunit. Make sure dependencies are installed in this sub directory.

stuartcusackie · 2022-07-28T20:23:34Z

I'm afraid I might have to leave it there because I don't use Docker, and I stopped using Vagrant a while back. I'm using Laragon on Windows. I don't think I'll figure this out anytime soon.

Sorry! I'm a little overloaded with other work at the moment.

stuartcusackie · 2022-07-28T20:52:42Z

I fixed up the asset reference and missing dependency there by the way.

stuartcusackie · 2022-07-29T09:44:46Z

My apologies, your instructions for testing worked perfectly fine on my environment. I have adjusted the tests now and they pass, though I'm not quite sure if I have everything covered.

ncla · 2022-07-29T10:05:24Z

Good to hear. I can take this for a spin over the weekend and maybe help out with tests.

riasvdv · 2022-07-31T09:32:16Z

src/Listeners/UpdateResponsiveReferences.php

+            return;
+        }
+
+        Log::info('UpdateResponsiveReferences listener triggered.');


Can you remove this log statement?

riasvdv · 2022-07-31T09:33:24Z

@ncla It looks good to me, if you can have another look and test it out then I'll merge 👍

ncla · 2022-07-31T10:22:50Z

I would like to suggest writing more/different tests as the current ones test only asset changes, not reference changes.

You can a look at how Statamic tests reference changing over here: https://github.com/statamic/cms/blob/8f18f78375a31a6f6e342b8410e3ce4aea529eed/tests/Listeners/UpdateAssetReferencesTest.php#L52-L89

Trying to whip a test with responsive field right now.

stuartcusackie · 2022-07-31T12:19:44Z

I would like to suggest writing more/different tests as the current ones test only asset changes, not reference changes.

You can a look at how Statamic tests reference changing over here: https://github.com/statamic/cms/blob/8f18f78375a31a6f6e342b8410e3ce4aea529eed/tests/Listeners/UpdateAssetReferencesTest.php#L52-L89

Trying to whip a test with responsive field right now.

Let me know if I can do anything and how many additional tests you would like.

ncla · 2022-07-31T14:28:17Z

I think I have been writing a test without actually testing manually in browser if it works. 😅

I see the log message from UpdateResponsiveReferences class in laravel.log. But it updates incorrectly. When I save the collection manually the content data is saved like this:

image:
  src:
    - 'assets::tttttt.png'

However when I change filename through asset browser, it saves the data like this:

image:
  src.0: 'assets::eeeee.png'

My blueprint looks like this:

-
        handle: image
        field:
          use_breakpoints: false
          allow_ratio: true
          allow_fit: true
          breakpoints:
            - sm
            - md
            - lg
            - xl
            - 2xl
          restrict: false
          allow_uploads: true
          display: Image
          type: responsive
          icon: assets
          listable: hidden
          instructions_position: above

Am I tripping? Can you verify? 😅

ncla · 2022-07-31T14:31:51Z

On the same blueprint I have another, regular asset field that does update correctly. Of course however that is handled by Statamic's own UpdateAssetReferences class.

stuartcusackie · 2022-07-31T16:36:48Z

I am seeing the same behaviour.

If the responsive field only has a single asset selected then renaming the file works fine.

hero_image:
  src: 'images::image-18-renamed.jpg'

If the responsive field has multiple files selected then I am seeing things like
hero_image:

hero_image:
  src.0: 'images::image-18-renamed.jpg'
  'md:src.0': 'imagef-17-renamed.jpg'

Fairly certain this is down to the DataReferenceUpdater@updateArrayValue() function.

We might need a custom update function. I'll see what I can tinker together here...

ncla · 2022-07-31T17:02:31Z

Yea I think you may also need to use more methods e.g. similar to what AssetReferenceUpdater@updateNestedFieldValues does which handles Replicators, Grids. Yours currently I think only handles top level values.

stuartcusackie · 2022-07-31T17:23:51Z

I haven't got the brainpower for this today, and I'm away from tomorrow. I can check next weekend but hopefully somebody with more Statamic data experience than myself can figure it out.

ncla · 2022-07-31T17:25:12Z

No rush, really. Could you add access for me to your repository? I think I may be able to add a test soon.

stuartcusackie · 2022-07-31T17:27:02Z

Invite sent ;)

stuartcusackie · 2022-07-31T18:55:56Z

Just some notes. The Arr::dot() call in DataReferenceUpdater@updateArrayValue()DataReferenceUpdater is adding a .0 to the end of each responsive src key.

Default code:

$fieldData = collect(Arr::dot(Arr::get($data, $dottedKey, [])));

Output:
{"src.0":"images::image1.jpg","md:src.0":"images::image2.jpg"}

Modified:

$fieldData = collect(Arr::get($data, $dottedKey, []));

Output:
{"src":["images::image1.jpg"],"md:src":["images::image2.jpg"]}

This produces the correct responsive src keys, but each file is wrapped in array tags, and the comparisons / transformations don't match anymore. It probably just requires a few more simple modifications but I can't figure them out right now.

One idea would be to keep the dot function and remap all of the collection keys on $fieldData to remove the zeroes.

Will revisit if someone doesn't beat me to it :)

stuartcusackie · 2022-07-31T19:16:42Z

Based on my last comment I have gone with the approach of remapping the $fieldData collection keys. It seems to work but maybe not ideal? I can't think of another way to do it.

So I guess this all needs to be tested with responsive fields that have a single image, and ones that have multiple images.

ncla · 2022-07-31T19:53:03Z

Added a test, took me a while.. 😅 something like this.

I had to boot up addon events in ServiceProvider otherwise I am not sure how to enable events during testing.

Some of the tests in AssetReferenceTest especially it_can_move_an_image and it_can_rename_an_image now fail for me locally, throw exception. This is due to booting the events. UpdateAssetReferences has woken up and triggers now on asset renames/location changes, but cannot find any fields, content files (?) associated with the asset, so it error outs, might be a bug with CMS? Not sure yet.

Not sure why it didn't fail on GitHub test run. Maybe cache from test bench that has old content files, blueprints?

Error: Call to a member function fields() on null

/var/www/html/statamic-responsive-images/vendor/statamic/cms/src/Data/DataReferenceUpdater.php:76
/var/www/html/statamic-responsive-images/vendor/statamic/cms/src/Data/DataReferenceUpdater.php:62
/var/www/html/statamic-responsive-images/vendor/statamic/cms/src/Listeners/UpdateAssetReferences.php:43
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Collections/Traits/EnumeratesValues.php:263
/var/www/html/statamic-responsive-images/vendor/statamic/cms/src/Listeners/UpdateAssetReferences.php:44
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Events/CallQueuedListener.php:107
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:36
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Container/Util.php:41
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:93
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:37
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Container/Container.php:651
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Bus/Dispatcher.php:128
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:141
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:116
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Bus/Dispatcher.php:132
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Queue/CallQueuedHandler.php:119
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:141
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:116
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Queue/CallQueuedHandler.php:121
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Queue/CallQueuedHandler.php:69
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Queue/Jobs/Job.php:98
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Queue/SyncQueue.php:43
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Queue/Queue.php:57
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php:591
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php:515
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php:441
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php:249
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Foundation/helpers.php:450
/var/www/html/statamic-responsive-images/vendor/laravel/framework/src/Illuminate/Foundation/Events/Dispatchable.php:14
/var/www/html/statamic-responsive-images/vendor/statamic/cms/src/Assets/Asset.php:457
/var/www/html/statamic-responsive-images/vendor/statamic/cms/src/Assets/Asset.php:562
/var/www/html/statamic-responsive-images/tests/Feature/AssetReferenceTest.php:60

To be continued.

ncla · 2022-08-04T20:41:17Z

I was writing a test for the fix you applied in last commit and I was thinking how come just single value for src is saved as an array, like so:

image:
  src:
    - 'assets::tttttt.png'

AFAIK it is not possible to have multiple images for a breakpoint. However you can have multiple breakpoints with single image in each.

If you save content file like this manually through code editor

src: 'images::image-18-renamed.jpg'

In control panel it will work just fine. Once saved again, it makes it into an array. Is this maybe an old relic of this addon where it saved this data in arrays? Or is it just how YAML works? I am asking because maybe there is a need to fix this data saving.

Regardless if we do fix the fieldtype saving data, it is good idea to account for old, outdated data I think anyway, in this PR.

I got this confusion because I am writing this test and if I use the same way of storing entry data, then array value gets converted to a string value

        $entry = tap(Facades\Entry::make()->collection($collection)->data([
            'avatar' => [
                'src' => ['test_container::test.jpg'],
            ],
        ]))->save();

becomes

avatar:
  src: 'test_container::new-test2.jpg'

But I will work around it. Curious what is happening here though.

stuartcusackie · 2022-08-04T21:09:52Z

I did notice that too - Each breakpoint in the fieldData is an array that only contains a single image. I don't know why!

By the way, my last commit seems silly now. I am using str_replace to convert the dot-notation array. It would probably be better to remove the Arr::dot function altogether and remap the values instead of the keys.

But maybe I'll leave it until we understand more about this single item array anomaly...?

ncla · 2022-08-04T22:15:00Z

Never mind, my test was working correctly. Took another fresh look. For now I wrote the test to expect same structure after a file has moved, as such the test is currently failing.

Question remains: should the array be converted to a string value when updating references? In my opinion, structure should be kept consistent with how it is saved normally. No surprise.

Rias, maybe you have a clue why is the src saving as an array in content files?

…replicator)

ncla · 2022-08-09T21:36:20Z

Went ahead and updated the updateResponsiveValue method so tests pass. Next up test and handle Bard fieldtype and maybe some other edge cases.

By the way, my last commit seems silly now. I am using str_replace to convert the dot-notation array. It would probably be better to remove the Arr::dot function altogether and remap the values instead of the keys.

This was helpful, thanks.

stuartcusackie · 2022-08-10T09:56:07Z

I have lost track of what is going here but let me know if I can do anything.

ncla · 2022-08-10T14:49:46Z

I think all what is left is testing Responsive field within other nested fieldtypes + re-test manually, check and remove the initial tests, look around at statamic/cms repo for issues/PRs regarding this feature and see if we missed any edge cases. I don't think much is left so I don't mind finishing this off if you don't have much time on your hands.

Less repeating code in ResponsiveReferenceUpdater

…iable in ServiceProvider https://laravel.com/docs/9.x/events#event-discovery

ncla · 2022-08-10T18:58:21Z

src/ResponsiveReferenceUpdater.php

+        $referencesUpdated = 0;
+
+        $fieldData->transform(function ($value, $key) use (&$referencesUpdated) {
+            if (!str_ends_with($key, 'src')) {
+                return $value;
+            }
+
+            // In content files, the src value can be either string or array.
+            // We first handle the string value, and then handle the array value.
+            if (is_string($value) && $value === $this->originalValue()) {
+                $referencesUpdated++;
+                return $this->newValue();
+            }
+
+            // Handle array value.
+            if (is_array($value) && in_array($this->originalValue(), $value)) {
+                return array_map(function ($item) use (&$referencesUpdated) {
+                    if ($item === $this->originalValue()) {
+                        $referencesUpdated++;
+                        return $this->newValue();
+                    }
+
+                    return $item;
+                }, $value);
+            }
+
+            return $value;
+        });
+
+        if ($referencesUpdated === 0) {
+            return;
+        }


I kinda feel weird about this $referencesUpdated usage here but I don't have an idea right now on how to rewrite this to be cleaner.

Something like this maybe for example https://github.com/statamic/cms/blob/a7f989f3d56eff1b21f0f4d0472bbdf16418b7c4/src/Assets/AssetReferenceUpdater.php#L292-L313

ncla · 2022-08-10T19:09:07Z

Okay I think I am pretty happy with this apart from the comment I left about $referencesUpdated.

Compared to regular asset reference updating, we do not have to handle asset URLs in Markdown, Bard images, Link fieldtype. The most Responsive fieldtype can do is be part of a set (in Bard, Replicator). There is a test for Replicator case, but for Grid and Bard sets/rows I just tested manually and reference updating worked.

stuartcusackie · 2022-08-10T20:12:47Z

I don't think much is left so I don't mind finishing this off if you don't have much time on your hands.

Thank you sir. You seem to know your way around the Statamic stuff much better than myself. I am just a noob.

All sounds good so far!

ncla · 2022-08-18T12:17:51Z

New changes to asset updating dropped in yesterdays core update. Need to check if all is good. statamic/cms#6504

Respect config value for reference updating See statamic/cms#6504

riasvdv · 2022-08-27T10:37:55Z

Thanks for all the work on this so far! Let me know when it's ready for a new review and merging

ncla · 2022-08-27T10:47:28Z

@riasvdv it is ready to be reviewed.

My only last minor concern is how potentially this could break if there are changes to AssetReferenceUpdater because we are extending it. Other than that most of this PR is imitating, copying what the core is doing for regular assets. Mostly the meat is in ResponsiveReferenceUpdater as the responsive fieldtype stores data a bit differently than a standard asset fieldtype does.

Add asset reference updating.

69bd926

Fix asset reference and missing dependencies in test.

f0319d3

Fix tests.

0f33384

riasvdv approved these changes Jul 31, 2022

View reviewed changes

Remove logging

b84827b

Add custom value updater

5bb352b

Test reference changing in content file

c1d65ab

Test reference change when src is an array

3e4afcb

Add replicator test

4d5d1fc

ncla added 2 commits August 10, 2022 00:27

Handle string and array src values; handle nested field values (e.g. …

ea485a9

…replicator)

Update phpdoc

32634da

ncla added 3 commits August 10, 2022 21:35

Extend AssetReferenceUpdater instead of DataReferenceUpdater

57743cd

Less repeating code in ResponsiveReferenceUpdater

Remove these tests as they do not test reference changes

80508a4

subscribe method is not needed if listener is attached on $listen var…

6098d7f

…iable in ServiceProvider https://laravel.com/docs/9.x/events#event-discovery

ncla reviewed Aug 10, 2022

View reviewed changes

Update asset reference updating to support asset deletion

05c7a50

Respect config value for reference updating See statamic/cms#6504

ncla added 2 commits September 26, 2022 19:48

Don't mind me just want to kick test work-flow running again

114fc62

Add back the dot

64b17ee

ncla merged commit 9636f3d into spatie:main Sep 26, 2022

ncla mentioned this pull request Oct 14, 2022

src values should be saved as string instead of an array in content files #172

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add asset reference updating. #149

Add asset reference updating. #149

stuartcusackie commented Jul 28, 2022 •

edited

Loading

ncla commented Jul 28, 2022 •

edited

Loading

stuartcusackie commented Jul 28, 2022

stuartcusackie commented Jul 28, 2022

stuartcusackie commented Jul 29, 2022

ncla commented Jul 29, 2022

riasvdv Jul 31, 2022

stuartcusackie Jul 31, 2022

riasvdv commented Jul 31, 2022

ncla commented Jul 31, 2022

stuartcusackie commented Jul 31, 2022

ncla commented Jul 31, 2022

ncla commented Jul 31, 2022

stuartcusackie commented Jul 31, 2022 •

edited

Loading

ncla commented Jul 31, 2022

stuartcusackie commented Jul 31, 2022

ncla commented Jul 31, 2022

stuartcusackie commented Jul 31, 2022

stuartcusackie commented Jul 31, 2022 •

edited

Loading

stuartcusackie commented Jul 31, 2022

ncla commented Jul 31, 2022

ncla commented Aug 4, 2022 •

edited

Loading

stuartcusackie commented Aug 4, 2022 •

edited

Loading

ncla commented Aug 4, 2022 •

edited

Loading

ncla commented Aug 9, 2022

stuartcusackie commented Aug 10, 2022

ncla commented Aug 10, 2022

ncla Aug 10, 2022

ncla commented Aug 10, 2022

stuartcusackie commented Aug 10, 2022

ncla commented Aug 18, 2022

riasvdv commented Aug 27, 2022

ncla commented Aug 27, 2022

Add asset reference updating. #149

Add asset reference updating. #149

Conversation

stuartcusackie commented Jul 28, 2022 • edited Loading

ncla commented Jul 28, 2022 • edited Loading

stuartcusackie commented Jul 28, 2022

stuartcusackie commented Jul 28, 2022

stuartcusackie commented Jul 29, 2022

ncla commented Jul 29, 2022

riasvdv Jul 31, 2022

Choose a reason for hiding this comment

stuartcusackie Jul 31, 2022

Choose a reason for hiding this comment

riasvdv commented Jul 31, 2022

ncla commented Jul 31, 2022

stuartcusackie commented Jul 31, 2022

ncla commented Jul 31, 2022

ncla commented Jul 31, 2022

stuartcusackie commented Jul 31, 2022 • edited Loading

ncla commented Jul 31, 2022

stuartcusackie commented Jul 31, 2022

ncla commented Jul 31, 2022

stuartcusackie commented Jul 31, 2022

stuartcusackie commented Jul 31, 2022 • edited Loading

stuartcusackie commented Jul 31, 2022

ncla commented Jul 31, 2022

ncla commented Aug 4, 2022 • edited Loading

stuartcusackie commented Aug 4, 2022 • edited Loading

ncla commented Aug 4, 2022 • edited Loading

ncla commented Aug 9, 2022

stuartcusackie commented Aug 10, 2022

ncla commented Aug 10, 2022

ncla Aug 10, 2022

Choose a reason for hiding this comment

ncla commented Aug 10, 2022

stuartcusackie commented Aug 10, 2022

ncla commented Aug 18, 2022

riasvdv commented Aug 27, 2022

ncla commented Aug 27, 2022

stuartcusackie commented Jul 28, 2022 •

edited

Loading

ncla commented Jul 28, 2022 •

edited

Loading

stuartcusackie commented Jul 31, 2022 •

edited

Loading

stuartcusackie commented Jul 31, 2022 •

edited

Loading

ncla commented Aug 4, 2022 •

edited

Loading

stuartcusackie commented Aug 4, 2022 •

edited

Loading

ncla commented Aug 4, 2022 •

edited

Loading