There's an issue over in the CSS Injector queue that has been tracked to a bug with the WYSIWYG module. The CSS Injector module uses the public:// protocol for its CSS files. When used on pages with a WYSIWYG module, the WYSIWYG textarea tries to call the .css file as well, and fails - leading to a bunch of watchdog errors.

The fix it to add support for the public:// protocol in the wysiwyg_get_css() function.

Patch coming in first comment...

-mike

Comments

StatusFileSize
new503 bytes

...and the patch.

Thanks,
-mike

Version:7.x-2.1» 7.x-2.x-dev
Status:Active» Needs review

Setting correct status.

The patch looks fine other than coding style (spacing) issues:

  • file_uri_scheme($filepath)==='public'

    should be

    file_uri_scheme($filepath) === 'public'
  • $files[]=file_create_url($filepath);

    should be

    $files[] = file_create_url($filepath);

Also, I would think that the identity operator (===) for the URI scheme isn't really necessary. Equality (==) will do, but that's just my observation of core style.

StatusFileSize
new506 bytes

@zhangtaihao - thanks for the patch review. An updated patch is attached.

Thanks,
-mike

Status:Needs review» Reviewed & tested by the community

Looks good.

Regarding my point in #4, I believe that public should be the major (if not the only) scenario apart from usual module/theme asset paths. When alternative stream wrappers (other than public, private, and temporary, of which only public should be used for assets) for drupal_add_css() really become a major use case I suppose a new issue should be opened to revisit this problem.

Status:Reviewed & tested by the community» Needs review

Thanks for moving this along @zhangtaihao!

Actually, is there any reason for why we're not using file_create_url($filepath) in both cases? file_create_url() should be able to handle both. Did you try whether we can simply replace the current line?

We should also make sure that the fully-qualified URI (including base URL; i.e., protocol, domain) is properly processed later on. I'm not sure whether there was a particular reason for just using base_path() previously. Did you investigate that?

Lastly, there's still trailing white-space in this patch.

Hmm... That's a good point. I'll have a look into the history of those lines in a bit.

StatusFileSize
new503 bytes

Updated patch removing the trailing white spaces.

-mike

Title:Proper handling of public:// protocol CSS filesFully process local CSS file paths
StatusFileSize
new445 bytes

Sorry I've been busy working on another project (which, incidentally, has also to do with file paths).

I have just gone through drupal_pre_render_styles() just to see how core renders style links from drupal_add_css(). The following are some observations:

  • CSS paths with type 'external' are used verbatim.
  • CSS paths with type 'file' can just be passed through to file_create_url().
  • As a principle, CSS paths are not checked with file_exists() in order to minimize load on the file system. The existence of a CSS file is the responsibility of whichever module that added it.

The only possible concerns are aggregation (i.e. 'preprocess'), browser targeting, and mixed-media style sheets, but we'll shelve those for now.

As such, this issue isn't even technically about public streams, but file paths in general, which should all be put through file_create_url() to get URLs. Given wysiwyg_get_css() is supposed to "Retrieve stylesheets for HTML/IFRAME-based editors", I suppose that's what we'll do. We'll also ignore checking whether resulting URL is empty, since adding a style sheet that doesn't have an external URL is daft anyway.

I've attached a patch. It's minimal, but it should do the job.

I think the current behavior is a result of file_create_url() doing something very different in Drupal 6. Back then it would rewrite all urls differently based on public/private files being in use. The Drupal 7 version of file_create_url() won't forcibly decide the "type" of file (private/public) since they now use different schemes and can coexist.

Thus, I think this should be applied to the D7 version, but not get backported to D6. Agree?

I believe so. In any case, public://css_injector/css_injector_1.css wouldn't occur in Drupal 6, so I think this is purely an issue triggered by stream wrappers in D7.

The patch in #10 makes sense to me.

-mike

StatusFileSize
new664 bytes

Since external paths are accepted by file_create_url() as well, why not pass everything but 'inline' through it too? That would allow altering all the paths if needed.

Still, there are quite a lot of places where we just use base_path(), mostly for plugin files.

Status:Needs review» Reviewed & tested by the community

While I would personally have targeted 'file' and 'external' specifically, #14 is perfectly fine, assuming drupal_add_css() is intended for files that can be passed through file_create_url() and that 'inline' is the only foreseeable exception.

As for other occurrences of base_path(), I would argue that, for now, they are used correctly in plugin files since it is the most efficient and system-agnostic method of passing assets through (this assessment may change in Drupal 8 with Assetic).

Issue summary:View changes

Dear TwoD, could you commit this please? :)

Status:Reviewed & tested by the community» Fixed

Oki, done!

The 7.x-2.x-dev snapshot will be updated within 12hrs and this will be part of the next Wysiwyg 7.x-2.x release.
I've not backported it to 6.x-2.x since there is no hook_file_url_alter() in D6.

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.