#2024 closed enhancement (fixed)
bp_core_override_adminbar_css() should no longer be used
Reported by: | TobiasBg | Owned by: | |
---|---|---|---|
Milestone: | 1.5 | Priority: | normal |
Severity: | Version: | ||
Component: | Core | Keywords: | has-patch dev-feedback |
Cc: |
Description
While investigation #2023, I found the function "bp_core_override_adminbar_css()" in bp-core/bp-core-cssjs.php.
This function is not needed and has drawbacks:
- In all cases, where this function would print its output (CSS commands), the corresponding HTML is not even there (see bp_core_admin_bar() in bp-core/bp-core-adminbar.php).
- The function inserts a HTML <style> element into the <body> of the page, thus making the page invalid HTML/XHTML.
Therefore, this function is not necessary and should be deprecated.
The attached patch removes the add_action call that loads the function, and adds a @deprecated comment (as I did not find another common practice of deprecating functions, like a bp-deprecated.php).
Attachments (3)
Change History (16)
#1
in reply to:
↑ description
;
follow-up:
↓ 2
@
15 years ago
- Priority changed from major to trivial
Deprecated functions are either removed completely and added to the BuddyPress Backwards Compatibility plugin, or have their guts replaced with a call to the new/replacement/alternative function. For instance:
function bp_core_override_adminbar_css() { newer_awesomer_function(); }
Given that this function is not used elsewhere, removal is the most likely course of action.> This function is not needed and has drawbacks:
- In all cases, where this function would print its output (CSS commands), the corresponding HTML is not even there (see bp_core_admin_bar() in bp-core/bp-core-adminbar.php).
Given that bp_core_override_adminbar_css() is not used elsewhere, and is handled in bp_core_admin_bar(),removal is the most likely course of action.
- The function inserts a HTML <style> element into the <body> of the page, thus making the page invalid HTML/XHTML.
The validation issues could have been fixed by hooking into wp_head
, instead of wp_footer
.
#2
in reply to:
↑ 1
@
15 years ago
Sorry for the bad copy/paste job ;). This:
Given that this function is not used elsewhere, removal is the most likely course of action.> This function is not needed and has drawbacks:
- In all cases, where this function would print its output (CSS commands), the corresponding HTML is not even there (see bp_core_admin_bar() in bp-core/bp-core-adminbar.php).
should have just read
This function is not needed and has drawbacks:
- In all cases, where this function would print its output (CSS commands), the corresponding HTML is not even there (see bp_core_admin_bar() in bp-core/bp-core-adminbar.php).
#4
@
15 years ago
@cnorris23:
I don't yet see what you want my feedback on.
Do you agree that the function is obsolete and should be deprecated?
And do you then want it removed instead of marked as "deprecated" the way I did in the patch? I can surely provide a patch for that, too.
#5
@
15 years ago
- Keywords dev-feedback added; reporter-feedback removed
Sorry about that. I think it was late when I wrote that, and at the time, I thought I was making sense :). I'm not totally sure on my logic for putting reporter-feedback in, but nonetheless, it did require you to respond. I do agree that's it at least appears obsolete. However, I only jumped in to BP during 1.2 development (I've never used anything less), so I can't be totally sure on it's status. Just from watching Trac and examining code, deprecated functions seem to be handled as I stated above, and since the functionality in bp_core_override_adminbar_css()
has either been replaced or no longer needed, IMHO it should just be removed.
Adding dev-feedback to make sure we're not crazy.
#6
@
15 years ago
bp_core_override_adminbar_css() is called when BP_DISABLE_ADMIN_BAR is defined or when the "Hide admin bar for logged out users?" option is checked under the "BuddyPress > General" settings admin page.
I don't think this function should be removed due to the option in the BP general settings page. It does serve a purpose.
I've created another ticket addressing the [style] tag problem - #2153, which addresses Tobias' main issue.
#7
@
15 years ago
r-a-y: Technically speaking you are correct in your first paragraph, that the function is executed when one of the two situations occurs.
However, please take a look at what the function then actually does: It adds some CSS for the CSS selector #wp-admin-bar.
However, the matching HTML element is only included if both of your mentioned situations do *not* occur. (here)
So, two problems:
- The function adds CSS that will never match a selector, because of exactly opposing condition checks (for the constant or for the setting from the admin page).
(S, it does *not* really serve purpose, and that actually is the main issue I'm addressing here.)
- The function adds the CSS in the body (which is invalid HTML and which you address in your ticket #2153).
Moving it to wp_head as you suggest would fix the second issue, however we would still be in a situation where we add CSS to the page that is totally useless, because it will *by design* never affect anything.
Do you now understand why the function should be removed?
#8
@
15 years ago
Tobias,
You're right. I didn't look at the bp_core_admin_bar() function. It will return false either in the define or the site option.
Will cosign you on this!
#9
@
14 years ago
- Component set to Core
- Milestone changed from 1.3 to 1.2.4
- Priority changed from trivial to normal
Attached is a fresh patch for the 1.2 branch: 2024-removal-1.2-branch.patch
Described behavior is still current, as the function still includes CSS into the page at the wrong place and that CSS is never matching a selector.
patch deprecates bp_core_override_adminbar_css()