awcodes / filament-tiptap-editor

A Rich Text Editor plugin for Filament Forms.
MIT License
290 stars 79 forks source link

Fix a bug when the APP_URL not start with http #133

Closed robin-dongbin closed 1 year ago

robin-dongbin commented 1 year ago

$data['src'] has include /storage, use Storage::url() function an additional /storage will be added.

awcodes commented 1 year ago

Out of curiosity, why would $data['src'] include /storage?

robin-dongbin commented 1 year ago

Out of curiosity, why would $data['src'] include /storage?

I think maybe because of this? https://github.com/awcodes/filament-tiptap-editor/blob/de582f7abc0309b69b462a52cb77f94cd63d870d/src/Actions/MediaAction.php#L97C15-L97C15

And you can set APP_URL empty to test it

awcodes commented 1 year ago

Sorry, I should have been more clear, what steps are you taking that causes the issue to appear. I need to be able to recreate it to see if it's an issue in the codebase or not.

robin-dongbin commented 1 year ago

set APP_URL empty to test it

awcodes commented 1 year ago

That doesn't make any sense. Why would APP_URL ever be empty?

awcodes commented 1 year ago

Please provide steps to reproduce the issue in a normal situation.

robin-dongbin commented 1 year ago

That doesn't make any sense. Why would APP_URL ever be empty?

If you think that doesn't make any sense, and you have to set a real APP_URL, then this is doesn' make any sense: https://github.com/awcodes/filament-tiptap-editor/blob/de582f7abc0309b69b462a52cb77f94cd63d870d/src/Actions/MediaAction.php#L117C11-L117C11,

because it's always start with http.

robin-dongbin commented 1 year ago

If this you want to make this condition has any sense: https://github.com/awcodes/filament-tiptap-editor/blob/de582f7abc0309b69b462a52cb77f94cd63d870d/src/Actions/MediaAction.php#L117C11-L117C11

then there has Storage::url($data['src']); and return Storage::disk($component->getDiskName())->url($upload);, they are repeated calls, an additional /storage will be added.

robin-dongbin commented 1 year ago

For the sense you want, I don't want to set APP_URL, and I want to store only path without domain in database. when I get data, I want to handle it by myself

awcodes commented 1 year ago

But APP_URL is Laravel .env setting, it's not specific to this plugin or even Filament and the Storage facade depends on it as well as Filament and Livewire's upload functionality. Sorry, but I don't see where that would ever be the case.

With that said, this sounds like an extreme use case and you can create your own MediaAction to provide that functionality for your app.

I'm failing to see where this would be an issue for the majority of users.

I appreciate the PR, but without you providing a reproduction that would allow me to evaluate if this is a bug in the plugin I'm going to close this. Feel free to reopen with reproduction repo so I can see what you are actually expierencing as an issue.

robin-dongbin commented 1 year ago

@awcodes Then you can update it

     $source = str_starts_with($data['src'], 'http')
                ? $data['src']
                : config('app.url') . Storage::url($data['src']);

to

$source = $data['src'];

Because this condition will always be true

awcodes commented 1 year ago

It won't always be true. When an image is inserted from the modal it may not have http. But when an image is selected and the action is triggered to edit the modal it will start with http or https since it's getting read from the attribute on the html tag in Tiptap.

That's why I've asked for reproduction.

robin-dongbin commented 1 year ago

@awcodes Then you can set config filament-tiptap-editor.disk = local, you will see this bug

awcodes commented 1 year ago

That wouldn't work anyway since local doesn't have a publicly accessible URL and image's / links to file's have to have a URL that can be reached.

awcodes commented 1 year ago

Are you overriding Filament's FileUpload saveUploadedFileUsing() method by any chance?

robin-dongbin commented 1 year ago

In browser, if the assets is start with '/', then the browser will add current domain automately.

robin-dongbin commented 1 year ago

If you only set it public, it will always start with http, then the condition has no sense

robin-dongbin commented 1 year ago

I've tried my best to illustrate, I'm not a naitve speaker. If you're still not willing to test it, there's nothing I can do.