Let's not put logo URL in an inline background-image style, these can be cached forever and thus are very annoying to change. It should be trivial to make a ResourceLoader module that would just output '#p-logo a { background-image: url($wgLogo); }'.
(This affects Vector and Monobook skins.)
Version: 1.23.0
Severity: enhancement
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=30113
https://bugzilla.wikimedia.org/show_bug.cgi?id=71334
Sounds sensible, though maybe avoid adding a new module for it. Perhaps add it to the 'site' module.
On that note, we might also want to move usercssprefs into user (it makes sense to separate a.t.m. because one is embedded as only=styles). And user.groups is separate from 'site' to avoid having 'site' be user specific, it could perhaps be merged into 'user' though.
Change 98356 had a related patch set uploaded by Tholam:
Site logo loaded using UserCSSPrefsModule instead of inline
t.lam wrote:
How do I add it to the site module? I tried doing something similar to what's in the UserCSSPreferences but that didn't seem to work.
I didn't test the code below, but it should be enough to override the getStyles() method:
class ResourceLoaderSiteModule extends ResourceLoaderWikiModule {
…
public function getStyles( ResourceLoaderContext $context ) {
$styles = parent::getPages( $context );
$styles['all'][] =
"#p-logo a { background-image: url($wgLogo); }";
return $styles;}
…
}
(In reply to comment #4)
$styles = parent::getPages( $context );
This of course should be getStyles, silly me. And I'm missing
global $wgLogo;.
t.lam wrote:
I tried putting this
public function getStyles( ResourceLoaderContext $context ) {
global $wgLogo;
$styles = parent::getStyles( $context );
$styles['all'][] = "#p-logo a { background-image: url($wgLogo); }";
return $styles;
}
in the ResourceLoaderSiteModule class but it doesn't get loaded. I already got rid of the inline styling but now there's just no logo.
Blaaaghh.
I spent way too much time tracking this down.
This is because the module is not loaded at all if its isKnownEmpty() method returns true, and it does return true for ResourceLoaderWikiModule (of which ResourceLoaderSiteModule is a subclass) if none of the pages it's supposed to check exist (as returned by getPages() method).
Of course, on a new wiki none of these pages exist, so the module is not even loaded.
The simplest solution that makes this work is adding public function isKnownEmpty( ResourceLoaderContext $context ) { return false; } to ResourceLoaderSiteModule, but this looks like an awful hack. We might want to create a new module after all per comment 0, but I'd like Krinkle to comment on this.
(I knew this is going to cause trouble, and Krinkle, I told you so. :P )
t.lam wrote:
I'll just go ahead and do it both ways and see which one we like more in the end.
Change 98460 had a related patch set uploaded by Tholam:
Create ResourceLoaderLogoModule to load logo
I had a little chat about this with Krinkle on #wikimedia-dev today.
Basically, it looks like we'll go with the original approach, it just needs some fixes – and while we're at it, let's change the selector for logo from "#p-logo a" to something saner like ".mw-wiki-logo" (and add this class to the appropriate element in the skins themselves).
Sorry, but I don't have much time for Wikimedia things today/tomorrow, so let me just paste the log here:
<Krinkle> MatmaRex: What about the sitelogo thing? Why is extending
the wikmodule a problem?
<MatmaRex> well, we need to override isKnownEmpty(), which looks hacky
to me, and might not be the most efficient thing ever in general
<MatmaRex> and we need to combine logic for cache invalidation with
the existing one
<MatmaRex> both of these mean that we need to much internal knowledge
of RLWikiModule than i'm comfortable with when subclassing it like this
<MatmaRex> (unless i'm missing something obvious to you)
<Krinkle> MatmaRex: overriding isKnownEmpty is fine, because it
unconditionally adds the css rule. Unless you make that css rule optional, there is no need to implement any known-empty logic.
<Krinkle> As for the cache invalidation, that's just parent::() and
then return max( $parentMTime, wglogomtime )
<Krinkle> which you calculate using the value-hash-cache-timestamp
logic
<MatmaRex> we have no wglogomtime i'm aware of, unless we store the
time we last noticed it changing
<Krinkle> yes
<MatmaRex> ugh.
<Krinkle> that's what we do for all values we cache that don't have
timestamps.
<Krinkle> we do the same for the language globals in langdata module
<MatmaRex> sounds unpleasant, but alright
<MatmaRex> so we stick to adding the logo to ResourceLoaderSiteModule?
<Krinkle> and definition timestamp, and git version hash, etc. there
is no other way to calculate it without lots of spurious invalidations (e.g. you'd need an array of all files that can possibly affect this global, and then some, and hte max() of their files, which is terrible and doesn't even always work with git and wmf-config)
<Krinkle> MatmaRex: Yep
<MatmaRex> Krinkle: while we're at it, let's change "#p-logo a" to
".mw-logo", hm?
<MatmaRex> .mw-wiki-logo, rather
<Krinkle> MatmaRex: Be sure to document that this means we will now
always add a request for modules=site&only=styles on every page, where previously this only happened on wikis that enabled sitejs/css and actually created 1 or more wiki pages
<Krinkle> MatmaRex: Sounds good
<MatmaRex> document where?
<Krinkle> MatmaRex: Commit message
<Krinkle> MatmaRex: Also, given the above, this will now depend on
https://gerrit.wikimedia.org/r/#/c/95463/
<MatmaRex> ah, duh. sure
<MatmaRex> hmm?
<Krinkle> MatmaRex: If not, you need to change OutputPage to take
these new factors into account, but probably easier to make it dependent on https://gerrit.wikimedia.org/r/#/c/95463/
<MatmaRex> why would it? it seemed to work when i tested very biriefly
<Krinkle> because we need to load site module unconditionally
<Krinkle> MatmaRex: Delete your local site js/css pages
<Krinkle> and try again :)
<MatmaRex> i have none. it loaded the site module when i changed
isKnownEmpty to return false, i think
<Krinkle> MatmaRex: OK, in addition, disable the wg variarbles that
enable it.
<Krinkle> They are false on a new wiki by default.
<MatmaRex> i'mmm pretty sure they're true by default
<MatmaRex> wgUseSiteJs / Css?
<MatmaRex> or something
<MatmaRex> ok anyway, will have to check this
<Krinkle> MatmaRex: okay, they weren' always true by default.
Whatever, the point is, OutputPage has some harccoded logic beyond isKnownEmpty that will break your change if you don't fix it.
<MatmaRex> got it, thanks
<Krinkle> Which is ugly, and why I remove it with
https://gerrit.wikimedia.org/r/#/c/95463/
Question: will this solve the problem of MediaWiki links published in Facebook or Google+ featuring the "Powered by MediaWiki" image at the footer (in pages without images added by editors) because these services can't find any other image in the page?
If this bugfix would make those services show the logo of the MediaWiki instance by default, this would be a great collateral effect. If not, then I might file a new enhancement request.
No, it won't; that would (to my understanding) require inlining the logo URL in the HTML output, but in a different way, or coming up with some magical solutions. That's already filed as bug 30113.
Change 162000 had a related patch set uploaded by Krinkle:
Use mw-wiki-logo class instead of inline background-image
Change 162001 had a related patch set uploaded by Krinkle:
Use mw-wiki-logo class instead of inline background-image
Change 98356 merged by jenkins-bot:
Set site logo url in ResourceLoaderSiteModule instead of inline styles
Change 162000 merged by jenkins-bot:
Use mw-wiki-logo class instead of inline background-image
Patches reverted in:
https://gerrit.wikimedia.org/r/#/c/163020/
and
https://gerrit.wikimedia.org/r/#/c/163023/
because of Bug 71334.
-> back to new
The patch needs to use a separate RL module rather than being included in the site module.
Just for the record, I am questioning my initial reasoning for this. My current opinion is that logo should just use <img src=$wgLogo alt=$wgSitename />, and who cares about caching forever.
Change 163043 had a related patch set uploaded by Krinkle:
Set site logo in mediawiki.skinning.interface module instead of inline styles
Change 163043 merged by jenkins-bot:
Set site logo in mediawiki.skinning.interface module instead of inline styles
Change 163291 had a related patch set uploaded by Krinkle:
Use mw-wiki-logo class instead of inline background-image
Change 162001 merged by jenkins-bot:
Use mw-wiki-logo class instead of inline background-image
Change 163291 merged by jenkins-bot:
Use mw-wiki-logo class instead of inline background-image
Re-merged into core, and patched for the tarball skins that display a logo (MonoBook and Vector).