search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

CKEditor: Recoverable fatal error when adding a link to an image #3384

Closed klonos closed 5 years ago

klonos commented 5 years ago

We have added the ability to link images added in CKEditor in #1586. I gave that a spin just now, and I got a WSOD. The following were logged:

Notice: Undefined index: scheme in filter_format_editor_link_form() (line 402 of /core/modules/filter/filter.pages.inc).

Recoverable fatal error: Argument 1 passed to file_usage_add() must be an instance of File, boolean given, called in /core/modules/filter/filter.module on line 1701 and defined in file_usage_add() (line 684 of /core/includes/file.inc).

(notice was logged twice, fatal only once)

Steps to reproduce (if reporting a bug)

  1. Create a page or a post
  2. Add a title
  3. Add an image in the WYSIWYG
  4. Click on the link button in the CKEditor toolbar
  5. Add a URL. like https://google.com and save

  6. Click the source button on the editor toolbar ->

    <p><a data-file-id="null" href="https://google.com" text=""><img alt="" data-file-id="1" height="344" src="/files/inline-images/pic.jpeg" width="344" /></a></p>

    👍

  7. Click "Save" to save the page/post

Actual behavior (if reporting a bug)

screen shot 2018-11-16 at 11 49 11 am

  1. "manually" visit the home page...

    • the content has actually been created 👍
    • the image is added 👍
    • the image is linked to the URL you entered in step 5 above 👍
    • there was no path alias created 👎

  2. Edit the content.

  3. Check the "URL settings" vertical tab -> the setting to create automatic alias is disabled 👎

Expected behavior (if reporting a bug)

bradbulger commented 5 years ago

I get the undefined index error on "scheme" and "status" adding an absolute URL as the link value of a normal text link

klonos commented 5 years ago

Thanks @bradbulger ...I was able to reproduce that:

  1. Create a page or a post
  2. Add a title
  3. Enter some text and add a link to an external URL like https://www.google.com
  4. Save -> no errors 👍
  5. Check the error log -> all clear 👍
  6. Create a page or a post
  7. Add a title
  8. Enter some text and add an absolute link to an internal URL like http://test.site/admin/reports/status
  9. Save -> no errors 👍
  10. Check the error log -> errors 👎:

Notice: Undefined index: scheme in filter_format_editor_link_form() (line 402 of /core/modules/filter/filter.pages.inc).

Notice: Undefined index: status in filter_format_editor_link_form() (line 410 of /core/modules/filter/filter.pages.inc).

olafgrabienski commented 5 years ago

Adding an absolute URL as the link value of a normal text link on a test site, I see no errors, no matter if the absolute URL points to an external URL or to an internal page. Maybe a question of the PHP version? On my test site it's PHP 5.6.32.

klonos commented 5 years ago

@olafgrabienski adding a link to an image results in a fatal error (see screenshot in issue summary). Adding a link to text does not show any visible errors, but if you check the system log in /admin/reports/dblog, you will see 2 php notices, as per my previous comment.

Tested both use cases on php 7.1.24

olafgrabienski commented 5 years ago

if you check the system log in /admin/reports/dblog, you will see 2 php notices

That's what I checked. Strangely, there were no notices in the DBlog.

klonos commented 5 years ago

I have switched my environment to php 5.6.38, and I was able to verify both the error when adding a link to an image, as well as the notices in the log when adding absolute internal links to text.

I am using Backdrop 1.x on Apache 2.4.33 and MariaDB 5.7.22.

olafgrabienski commented 5 years ago

Still only testing text links at the moment: I've changed PHP from 5.6.32 to 7.1.12, and I still don't see any errors in the Recent log messages page after adding an absolute internal link to text in CKEditor. I'm on a remote hosting, so I've limited knowledge of the environment; information from the status report page:

bradbulger commented 5 years ago

I can't recreate the error now. I'm not aware of having done anything significant to my environment, so I don't know. I might have enabled or disabled some other module that's having some effect, but looking at the log I don't see anything that seems relevant. The main thing that has happened between then and now is stopping and restarting the Docker image the site runs in. So I guess it's not impossible that something has changed and I've forgotten about it.

klonos commented 5 years ago

@olafgrabienski and @bradbulger can you guys at least confirm the original bug report using the steps I have posted in the issue summary (adding a link to an image and saving the node results in fatal error)?

bradbulger commented 5 years ago

I get the fatal error from file_usage_add() but not the undefined index error. That's when I upload an image file and use a full absolute URL for the link value. The source looks like what's in the summary.

It displays thumbnails of some existing images, but I couldn't figure out how to use them. Clicking on them does nothing visible. I tried pasting the image location into the "Select from library" field, which seems to work. That's a full absolute URL, not a relative one. I was able to save that without errors.

I tried changing that path into a Backdrop relative path - /files/styles/etc - that didn't work. Including the base path of the installation does work.

If I edit the page without editing the image I get no error. The link is there. I can edit the link subsequently without error.

olafgrabienski commented 5 years ago

@klonos Sure! I also only get the file_usage_add() related error in my environment it's a "TypeError":

TypeError: Argument 1 passed to file_usage_add() must be an instance of File, boolean given, called in /(...)/core/modules/filter/filter.module on line 1701 in file_usage_add() (line 684 of /(...)/core/includes/file.inc).

The rest was as described in the issue description.

Interesting: When I saw the page with the error, and before 'manually' going to another page, the URL path in the browser address bar was still node/add/post.

Graham-72 commented 5 years ago

I think the problem is that the line of source code shown previously <p><a data-file-id="null" href="https://google.com" text=""><img alt="" data-file-id="1" height="344" src="/files/inline-images/pic.jpeg" width="344" /></a></p>

should not include

data-file-id="null"

since it is not linking to a data file.

I have submitted a PR 2402 to make a small addition to the CKEditor backdroplink plugin script.

olafgrabienski commented 5 years ago

@Graham-72 Thanks for the PR! I've had a quick look at the sandbox site and was able to add an absolute link to an image in CKEditor and to save the post. I saw however two notices in the DBlog:

Notice: Undefined index: scheme in filter_format_editor_link_form() (line 402 of /home/qa/github/backdrop/backdrop/pr/2402/core/modules/filter/filter.pages.inc). Notice: Undefined index: status in filter_format_editor_link_form() (line 410 of /home/qa/github/backdrop/backdrop/pr/2402/core/modules/filter/filter.pages.inc).

Graham-72 commented 5 years ago

@olafgrabienski thanks for this. I will try and see what is causing these notices.

Graham-72 commented 5 years ago

@olafgrabienski I have added to my PR so that default values are set in filter.pages.inc. Hopefully this will fix the issue.

olafgrabienski commented 5 years ago

@Graham-72 Tested again, there are no notices anymore.

klonos commented 5 years ago

Sorry @Graham-72 I had not enabled errors on my local when I was fling this issue. I have enabled it now and have tested again. Here'e what I get:

Notice: Undefined index: scheme in filter_format_editor_link_form() (line 402 of /core/modules/filter/filter.pages.inc).
Notice: Undefined index: status in filter_format_editor_link_form() (line 410 of /core/modules/filter/filter.pages.inc).
TypeError: Argument 1 passed to file_usage_add() must be an instance of File, boolean given, called in /core/modules/filter/filter.module on line 1701 in file_usage_add() (line 684 of /core/includes/file.inc).

After applying the fix from your PR on my local, I only get the TypeError: Argument 1 ... error above. Still no joy 😞

Graham-72 commented 5 years ago

@klonos I found that once I had made sure that my browser was using the revised plugin script (by forcing it to clear its cache and hard reload) I was no longer getting the TypeError: Argument 1 ... error report. Could this be relevant?

stpaultim commented 5 years ago

I tested the sandbox and am able to accomplish the desired task without generating errors. However, if I create a patch from the pull request and apply it to a local version of Backdrop (running in Lando) the patch does not fix the problem. I continue to get the same error message (see original report) and no path alias is created.

I created the patch by modifying the PR path to: https://github.com/backdrop/backdrop/pull/2402.patch

And applying the patch to Backdrop: patch -p1 < 2402.patch

FYI: I tried applying the patch to both 1.11.2 and 1.11.3

stpaultim commented 5 years ago

I just spent over an hour trying to figure out why it's not working on my local machine. I do not have full solution, but I have info that I think will be helpful.

1) If I spin up a fresh installation of Backdrop and apply the patch immediately. It resolves the problem.

2) HOWEVER, if I spin up a fresh installation and attempt to create a link on an image in CKEditor and fail BEFORE I apply the patch. I can then apply the patch and clear the cache and the patch does not have an effect.

STEPS:

1) Spin up fresh installation of Backdrop 2) Attempt to link an image in CKEditor and try to save the node (generates error message) 3) Apply the patch 4) Clear the cache on the command line AND clear the cache in the UI 5) Add an image in CKEditor and create a link (if I look at source "data-file-id="null"" is still there) and I get an error when I try saving the node. 6) Error message in log = "TypeError: Argument 1 passed to file_usage_add() must be an instance of File, boolean given, called in /app/core/modules/filter/filter.module on line 1700 in file_usage_add() (line 684 of /app/core/includes/file.inc)."

I've tried this multiple times and have on at least 2 occasions applied the patch successfully before attempting to save an node and the patch has worked.

AND on at least 2 occasions it has failed if I first try to save a node with a linked image BEFORE applying the patch.

stpaultim commented 5 years ago

OK. @Radcliffe just hit me with the hard truth (that I missed in @Graham-72's comment) that clearing the cache can mean more than clearing the Drupal cache. Clearing the browser cache seems to resolve this issue. Which means that this resolves the issue.

We could include something in the release notes to warn users that they may need to clear the browser cache, but I suspect that is not necessary and might apply to other issues anyway.

I'm going to mark this RTBC (and ask forgiveness later if I'm wrong).

klonos commented 5 years ago

Yes!! 🎉 ...clearing browser caches does fix the issue. RTBC form me too 😄 👍

quicksketch commented 5 years ago

Thanks folks! Yeah a problem with CKEditor is that it loads the JavaScript via it's own mechanism AND it's inside of an iframe, so plugin changes don't show up right away. Seems like something we might want to try and figure out if we can cache-bust based on Backdrop version numbers or something, to prevent this problem for end-users that update.

In any case, I've merged https://github.com/backdrop/backdrop/pull/2402 into 1.x and 1.11.x. Thanks folks for the collaboration in getting this fixed!!