Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.