Posted by ultimike on September 24, 2012 at 8:17pm
7 followers
| Project: | Wysiwyg |
| Version: | 7.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | reviewed & tested by the community |
Issue Summary
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
#1
...and the patch.
Thanks,
-mike
#2
Patch solved it for me! Thanks! I got to this issue from #1547918: Page not found in log (public://) #1485444: Notice: page not found - public:/css_injector/css_injector_1.css.
#3
Setting correct status.
#4
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.#5
@zhangtaihao - thanks for the patch review. An updated patch is attached.
Thanks,
-mike
#6
Looks good.
Regarding my point in #4, I believe that
publicshould be the major (if not the only) scenario apart from usual module/theme asset paths. When alternative stream wrappers (other thanpublic,private, andtemporary, of which onlypublicshould be used for assets) fordrupal_add_css()really become a major use case I suppose a new issue should be opened to revisit this problem.#7
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.
#8
Hmm... That's a good point. I'll have a look into the history of those lines in a bit.
#9
Updated patch removing the trailing white spaces.
-mike
#10
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 fromdrupal_add_css(). The following are some observations:'external'are used verbatim.'file'can just be passed through tofile_create_url().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
publicstreams, but file paths in general, which should all be put throughfile_create_url()to get URLs. Givenwysiwyg_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.
#11
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?
#12
I believe so. In any case,
public://css_injector/css_injector_1.csswouldn't occur in Drupal 6, so I think this is purely an issue triggered by stream wrappers in D7.#13
The patch in #10 makes sense to me.
-mike
#14
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.
#15
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 throughfile_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).