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

WYSIWYG fails to render image shortcode if title parameter is empty #2489

Closed
1 task
purplespider opened this issue Oct 22, 2019 · 6 comments · Fixed by silverstripe/silverstripe-assets#474
Closed
1 task

Comments

@purplespider
Copy link
Contributor

purplespider commented Oct 22, 2019

To reproduce, have an image shortcode in a content area which has the title parameter set, but empty. e.g.:

[image src="/assets/image.jpg" title="" id="123" width="100" height="100"]

(The TagsToShortcode task produces shortcodes with empty title tags like this, if the original img tag had an empty title attribute.).

While the image will render correctly when viewing the page. It fails to render in the WYSIWYG editor, and instead, the shortcode is just displayed:

Note the 4 quote marks for the title parameter.

I've noticed that when a user inserts an image via the CMS and empties the Title field, the title attribute is just omitted from the shortcode altogether. So one alternative workaround could be to update the TagsToShortcode task to only add the title parameter if it isn't empty. But I'd imagine it's best to fix whatever is causing an empty title tag to cause it to not render in the WYSIWYG.

Pull request

@purplespider
Copy link
Contributor Author

For anyone else experiencing this issue after upgrading a site to Silverstripe 4, I've found the easiest fix is to just take a dump of the database (after the upgrade) and then do a find and replace to remove all instances of title=\"\".

I welcome discussion of whether the issue should be resolved by either:

  • Updating the TagsToShortcode task to only add the title parameter if it isn't empty.
  • Updating the WYSIWYG shortcode handling so it doesn't get tripped up by empty title tags.

@purplespider
Copy link
Contributor Author

Just upgraded a site from Silverstripe 3.7 to 4.6.1 and this is still an issue.

@micschk
Copy link

micschk commented Feb 23, 2021

I know I'm a bit late to the game, but I also just ran into this issue. Seems it's also the case with empty alt tags (probably any empty tag?). I'd say this should probably best be fixed in the WYSIWYG rendering instead of the TagsToShortcode task (where I fixed it for the time being).

QUICK FIX in TagsToShortcode:

if ($parsedFileID && $file) {
            ...
            $shortcode = str_replace('title=""', '', $shortcode);
            $shortcode = str_replace('alt=""', '', $shortcode);

            return $shortcode;
    ...
}

Note: there were some old image-paths containing '../../../'-like paths in the site I was migrating. These crashed the TagsToShortcode task because they were 'outside of the defined root'. As a quick fix I wrapped a try catch block around this part since these files should just be 'not found' instead of halting the task.

        try {
            $filesystem = $this->flysystemAssetStore->getPublicFilesystem();
            ... etc
        } catch (Exception $e){
            $parsedFileId = null;
            // else: remove any '../' from $fileID as these result in LogicException: Path is outside of the defined root
        }

@maxime-rainville
Copy link
Contributor

silverstripe/silverstripe-framework#9872 will have fixed this issue.

If not, let me know and I'll reopen.

@purplespider
Copy link
Contributor Author

Sorry @maxime-rainville, this doesn't look to be fully resolved yet. If the title attribute is empty, it now seems to change it to title="title", rather than title="".

To reproduce:

  1. Add [image src="/assets/image.jpg" title="" id="123" width="100" height="100"] to a Content field in the database.
  2. Open the page in the CMS, note that the title of the image is now set to "title", and is applied when the page is saved.

Reproduced in Silverstripe 4.9

@maxime-rainville
Copy link
Contributor

silverstripe/silverstripe-assets#474 should fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants