Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#14 | wysiwyg-local_css_file_paths-1793704-14.patch | 664 bytes | TwoD |
Comments
Comment #1
ultimike...and the patch.
Thanks,
-mike
Comment #2
radj CreditAttribution: radj commentedPatch 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.
Comment #3
rfaySetting correct status.
Comment #4
zhangtaihao CreditAttribution: zhangtaihao commentedThe patch looks fine other than coding style (spacing) issues:
should be
should be
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.Comment #5
ultimike@zhangtaihao - thanks for the patch review. An updated patch is attached.
Thanks,
-mike
Comment #6
zhangtaihao CreditAttribution: zhangtaihao commentedLooks 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 thanpublic
,private
, andtemporary
, of which onlypublic
should 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.Comment #7
sunThanks 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.
Comment #8
zhangtaihao CreditAttribution: zhangtaihao commentedHmm... That's a good point. I'll have a look into the history of those lines in a bit.
Comment #9
ultimikeUpdated patch removing the trailing white spaces.
-mike
Comment #10
zhangtaihao CreditAttribution: zhangtaihao commentedSorry 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
public
streams, 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.
Comment #11
TwoDI 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?
Comment #12
zhangtaihao CreditAttribution: zhangtaihao commentedI 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.Comment #13
ultimikeThe patch in #10 makes sense to me.
-mike
Comment #14
TwoDSince 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.
Comment #15
zhangtaihao CreditAttribution: zhangtaihao commentedWhile 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).Comment #16
darrell_ulm CreditAttribution: darrell_ulm commentedVerified that #14 works: https://drupal.org/node/1793704#comment-7205846
Comment #17
giorgio79 CreditAttribution: giorgio79 commentedDear TwoD, could you commit this please? :)
Comment #18
TwoDOki, 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.