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

Revert change to fix image href #5595

Merged
merged 1 commit into from
Aug 6, 2024
Merged

Revert change to fix image href #5595

merged 1 commit into from
Aug 6, 2024

Conversation

jcastroa87
Copy link
Member

WHY

BEFORE - What was wrong? What was happening before this PR?

href is broken by the change on this line

$column['disk'] = $column['disk'] ?? config('filesystems.default');

AFTER - What is happening after this PR?

href is working as normal

HOW

How did you achieve that, in technical terms?

Revert the change

Is it a breaking change?

No

How can we test the before & after?

Check is return the correct URL of column image

If the PR has changes in multiple repos please provide the command to checkout all branches, eg.:

git checkout "dev-branch-name" &&
cd vendor/backpack/crud && git checkout crud-branch-name &&
cd ../pro && git checkout pro-branch-name &&
cd ../../..

@pxpm
Copy link
Contributor

pxpm commented Aug 5, 2024

Hello @jcastroa87 can you point me what and when do things get broken ? What would be the expected output etc etc ?

From my understanding that line only does:

  • set disk from column if it exists
  • use the default disk from filesystem

What happened before ? There was no disk in the column ? It gets a different disk ?

Please post some screenshots or some code example so that I can understand the issue. The line you mention seems to be very innocent at a first look.

Cheers

@pxpm pxpm removed their request for review August 5, 2024 10:29
@pxpm pxpm assigned jcastroa87 and unassigned pxpm Aug 5, 2024
@pxpm pxpm added Possible Bug A bug that was reported but not confirmed yet. needs-more-info labels Aug 5, 2024
@jcastroa87
Copy link
Member Author

Sure, i send you this by chat early, but here is again.

The filedisk is by default.

Column params:

CRUD::addColumn([
'name' => 'path',
'type' => 'image',
'label' => 'Image',
'prefix' => 'imagecache/admin/',
'height' => '200px',
'width' => '200px',
]);

i try in CRUD 6.7.14, the result is:

array:16 [ // vendor/backpack/crud/src/resources/views/crud/columns/image.blade.php
"name" => "name"
"type" => "image"
"label" => "Image"
"prefix" => "imagecache/admin/"
"height" => "200px"
"width" => "200px"
"key" => "name"
"priority" => 1
"tableColumn" => true
"orderable" => true
"searchLogic" => true
"value" => "qwqw"
"radius" => "3px"
"temporary" => false
"expiration" => 1
"wrapper" => array:3 [
"element" => "a"
"href" => "http://bp.test/imagecache/admin/qwqw"
"target" => "_blank"
]
]

When i update (backpack/crud (6.7.14 => 6.7.24))
results now is:

array:17 [ // vendor/backpack/crud/src/resources/views/crud/columns/image.blade.php
"name" => "name"
"type" => "image"
"label" => "Image"
"prefix" => "imagecache/admin/"
"height" => "200px"
"width" => "200px"
"key" => "name"
"priority" => 1
"tableColumn" => true
"orderable" => true
"searchLogic" => true
"value" => "qwqw"
"radius" => "3px"
"temporary" => false
"expiration" => 1
"disk" => "local"
"wrapper" => array:3 [
"element" => "a"
"href" => "/storage/imagecache/admin/qwqw"
"target" => "_blank"
]
]

is full change the "href" value.

Cheers.

@pxpm
Copy link
Contributor

pxpm commented Aug 6, 2024

So the issue here is that we are now forcing a disk, while previously we would allow it to work without a disk. I should have spotted this, this is indeed a breaking change.

A tip for the future, when you want to show diffs of some text you can use ``` diff

image

- this was removed
+ this was added

In this case it would make it easier for me to spot the difference.

array:17 [ // vendor/backpack/crud/src/resources/views/crud/columns/image.blade.php
"name" => "name"
"type" => "image"
"label" => "Image"
"prefix" => "imagecache/admin/"
"height" => "200px"
"width" => "200px"
"key" => "name"
"priority" => 1
"tableColumn" => true
"orderable" => true
"searchLogic" => true
"value" => "qwqw"
"radius" => "3px"
"temporary" => false
"expiration" => 1
+ "disk" => "local"
"wrapper" => array:3 [
"element" => "a"
- "href" => "http://bp.test/imagecache/admin/qwqw"
+ "href" => "/storage/imagecache/admin/qwqw"
"target" => "_blank"
]
]

Cheers

@pxpm pxpm merged commit ebb278e into main Aug 6, 2024
9 checks passed
@pxpm pxpm deleted the fix-image-column-href branch August 6, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-more-info Possible Bug A bug that was reported but not confirmed yet.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants