Skip to:
Content

BuddyPress.org

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#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)

2024.patch (655 bytes) - added by TobiasBg 11 years ago.
patch deprecates bp_core_override_adminbar_css()
2024-removal.patch (804 bytes) - added by TobiasBg 11 years ago.
Patch removes unneeded function instead of just marking it
2024-removal-1.2-branch.patch (833 bytes) - added by TobiasBg 11 years ago.
Refreshed patch to remove unnecessary and erroneous function

Download all attachments as: .zip

Change History (16)

@TobiasBg
11 years ago

patch deprecates bp_core_override_adminbar_css()

#1 in reply to: ↑ description ; follow-up: @cnorris23
11 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 @cnorris23
11 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).

#3 @cnorris23
11 years ago

  • Keywords reporter-feedback added

#4 @TobiasBg
11 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.

@TobiasBg
11 years ago

Patch removes unneeded function instead of just marking it

#5 @cnorris23
11 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 @r-a-y
11 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 @TobiasBg
11 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 @r-a-y
11 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 @TobiasBg
11 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.

@TobiasBg
11 years ago

Refreshed patch to remove unnecessary and erroneous function

#10 @apeatling
11 years ago

  • Milestone changed from 1.2.4 to 1.3

#11 @TobiasBg
11 years ago

related as a duplicate: #2448

#12 @johnjamesjacoby
10 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [3279]) Fixes #2024, #2448 (branch). Also adds missing new lines. Props TobiasBg.

#13 @johnjamesjacoby
10 years ago

(In [3280]) Fixes #2024, #2448 (trunk). Also adds missing new lines. Props TobiasBg.

Note: See TracTickets for help on using tickets.