Opened 7 months ago
Last modified 3 months ago
#9210 reopened defect (bug)
Review the way we load 12.0.0 deprecated code
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Up Next | Priority: | normal |
Severity: | normal | Version: | 14.0.0 |
Component: | Core | Keywords: | dev-feedback needs-dev-note needs-docs |
Cc: | emaralive |
Description
While testing the 14.0.0 release, I noticed that when the first version used to install BuddyPress was 12.0.0, the 12.0.0 deprecated code wasn't loaded, although it was the case when performing a fresh install of BuddyPress (meaning the first version in this case is 14.0.0).
I decided to add a quick fix since it's not possible to edit a tag once it is created, I'm now committing back the change to trunk and the 14.0 branch so that we can discuss about it again and eventually improve/revert the change.
My goal was to avoid the risk of a fatal error as some user configs may still have a plugin not compatible with BP 12.0 active and generating deprecated notices.
Attachments (1)
Change History (20)
#3
@
7 months ago
Are we still going to review this or are the changes you made, final? I'm asking because I brought this up in a comment ticket 9075.
#4
@
7 months ago
You were right, we need to review it, so yes, let's try to find the best way to handle it.
Should we carry on including 12.0.0 deprecated code no matter what updated version is?
That's we're doing now. Previously we used to load the two latest versions deprecated code (unless the constant to load all deprecated code is set to true
).
This ticket was mentioned in PR #352 on buddypress/buddypress by @imath.
7 months ago
#5
- Keywords has-patch has-unit-tests added
Always Include 12.0 deprecated functions as soon as initial version is < 15.0.0.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/9210
#6
@
7 months ago
- Keywords dev-feedback added
I suggest to stop including 12.0 deprecated code by default as soon as the first version used to install BuddyPress is 15.0
@emaralive what's your opinion about it (and about the above PR)?
#7
@
7 months ago
- Cc emaralive added
@imath,
I offer the opposite thought, we should continue to include/load 12.0 deprecated code based on the fact that when I search the plugin directory for plugins that should be relevant to BuddyPress (site.url/wp-admin/plugin-install.php
) the result is approximately 30 pages with, let's say, 36 plugins per page begets a number of 1,088 potential plugins. The question, to me, is: How many plugins use functions that were deprecated with 12.0 that would cause fatal errors, if 12.0 deprecated code were not included/loaded?
Since the number of potential plugins is quite large, a smaller arbitrary sampling netted the following results (it's all I had time to process, at the moment):
Block, Suspend, Report for BuddyPress
Steps to reproduce:
- Activate plugin
- Switch/login to a non-administrator account, e.g., subscriber
- Visit another user's profile area who is also a non-administrator account, e.g., subscriber
Fatal error: Uncaught Error: Call to undefined function bp_core_get_user_domain() in /wp-content/plugins/bp-toolkit/includes/class-bp-toolkit-report.php:189 Stack Trace 1. BPTK_Report->add_profile_report_button() /wp-includes/class-wp-hook.php:324 2. WP_Hook->apply_filters() /wp-includes/class-wp-hook.php:348 3. WP_Hook->do_action() /wp-includes/plugin.php:517 4. do_action() /wp-content/plugins/buddypress/bp-templates/bp-nouveau/includes/members/template-tags.php:148 5. bp_nouveau_member_header_buttons() /wp-content/plugins/buddypress/bp-templates/bp-nouveau/buddypress/members/single/cover-image-header.php:28 6. require_once('/var/www/html/z...') /wp-includes/template.php:810 7. load_template() /wp-content/plugins/buddypress/bp-templates/bp-nouveau/includes/members/functions.php:341 8. /wp-content/plugins/buddypress/bp-templates/bp-nouveau in /wp-content/plugins/bp-toolkit/includes/class-bp-toolkit-report.php on line 189
BuddyPress Simple Events
Steps to reproduce:
- Activate plugin
- As adminstrator visit
site.url/wp-admin/options-general.php?page=bp-simple-events
- Select a non-administrator user role that coincides with an active user with same assigned user role, e.g., subscriber, and then "Save Roles"
- As administrator or user that has a user role selected in step 3, visit a user's profile area, e.g., administrator, or role from step 3
- Click on "Events" menu item
Fatal error: Uncaught Error: Call to undefined function bp_core_get_user_domain() in /wp-content/plugins/buddypress-simple-events/templates/members/single/profile-events-loop.php:33 Stack Trace 1. require() /wp-includes/template.php:812 2. load_template() /wp-content/plugins/buddypress/bp-core/bp-core-template-loader.php:225 3. bp_locate_template() /wp-content/plugins/buddypress/bp-core/bp-core-template-loader.php:67 4. bp_get_template_part() /wp-content/plugins/buddypress-simple-events/inc/pp-events-screens.php:15 5. pp_events_profile_screen() /wp-includes/class-wp-hook.php:324 6. WP_Hook->apply_filters() /wp-includes/class-wp-hook.php:348 7. WP_Hook->do_action() /wp-includes/plugin.php:517 8. do_action() /wp-content/plugins/buddypress/bp-templates/bp-nouveau/includes/template-tags.php:36 9. /wp-content/plugins/buddyp in /wp-content/plugins/buddypress-simple-events/templates/members/single/profile-events-loop.php on line 33
bbPress
Steps to reproduce:
- Activate plugin - default settings will suffice
- If no groups are created, enable groups component, create a group and enable forums for group.
- Visit
site.url//wp-admin/edit.php?post_type=forum
, you should notice text that indicates a critical error has occurred where the Group description should be. - Clicking the group link will also provide proof that a critical error has occurred.
Fatal error: Uncaught Error: Call to undefined function bp_get_group_permalink() in /wp-content/plugins/bbpress/includes/extend/buddypress/groups.php:1499 Stack Trace 1. BBP_Forums_Group_Extension->maybe_map_permalink_to_group() /wp-content/plugins/bbpress/includes/extend/buddypress/groups.php:1603 2. BBP_Forums_Group_Extension->post_type_link() /wp-includes/class-wp-hook.php:326 3. WP_Hook->apply_filters() /wp-includes/plugin.php:205 4. apply_filters() /wp-includes/link-template.php:375 5. get_post_permalink() /wp-includes/link-template.php:201 6. get_permalink() /wp-admin/includes/class-wp-posts-list-table.php:1548 7. WP_Posts_List_Table->handle_row_actions() /wp-admin/includes/class-wp-posts-list-table.php:1073 8. WP_Posts_List_Table->_column_title() /wp-admin/includes/class-wp-list-table.php:1793 9. /wp-admin/inclu in /wp-content/plugins/bbpress/includes/extend/buddypress/groups.php on line 1499
As noted previously, this is just a small sample that would not be considered statistically significant, but they do indicate that exclusion of 12.0 deprecated code would have undesirable consequences. So, let's do this; have this as a discussion topic (agenda item) for the upcoming dev-chat that should occur on July 31, 2024. In the meantime, given that I have time, I believe it would be pertinent to generate a list of plugins from the potential 1088 that would cause grief to end users due to the exclusion of 12.0 deprecated code (IOW, extend the list of 3, if there are others and improve upon the "steps to reproduce"). Thoughts?
Furthermore, I haven't had a chance to look at the PR, in depth, so I'll try to do so during this upcoming week (week 31 of 2024).
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
6 months ago
#9
@
6 months ago
- Keywords has-testing-info added
@imath
cc: @espellcaste (it appears that you have been requested to review PR 352)
I did a cursory review of the history for the handling/loading of deprecated functions going back to #8687 in an attempt to gain a better understanding of the strategy to "don't load deprecated functions for new installs" and the jury is still out in deliberation (to be reviewed in greater detail, when time permits). Notwithstanding the deliberation, it was agreed upon during the last dev-chat (July 31, 2024) to continue loading 12.0 deprecated functions for "new installs" until BP 16.0.0. Furthermore, there should be a documented "Deprecation Policy" so that individuals don't have to dig through tickets to find rhyme and reason for why things are done the way they are done in regard to deprecation.
Additionally, I ran some ad hoc tests against the bp_get_deprecated_functions_versions()
function both with and without the patch/PR 352 and the results are as follows:
Without Patch
$current_major_version = 14.0
$initial_version | bp_get_deprecated_functions_versions() | passed |
---|---|---|
14.0 | 12.0 | ✔️ |
12.0 | 12.0, 14.0 | ✔️ |
11.0 | 12.0, 14.0 | ✔️ |
0 | 12.0, 14.0 | ✔️ |
$current_major_version = 15.0
NOTE
: The $deprecated_functions_versions
array was amended to include 15.0
$initial_version | bp_get_deprecated_functions_versions() | passed |
---|---|---|
15.0 | 12.0 | ✔️ |
14.0 | 15.0 | ❌ |
12.0 | 14.0, 15.0 | ❌ |
11.0 | 14.0, 15.0 | ❌ |
0 | 14.0, 15.0 | ❌ |
With Patch/PR 352
$current_major_version = 14.0
NOTE
: Test for upgrade from 11.0
fails with fatal error in live situation
Fatal error: Cannot redeclare bp_core_create_root_component_page() (previously declared in /wp-content/plugins/buddypress/bp-core/deprecated/12.0.php:554) in /wp-content/plugins/buddypress/bp-core/deprecated/12.0.php on line 554
$initial_version | bp_get_deprecated_functions_versions() | passed |
---|---|---|
14.0 | 12.0 | ✔️ |
12.0 | 12.0, 14.0 | ✔️ |
11.0 | 12.0, 12.0, 14.0 | ❌ |
0 | 12.0, 14.0 | ✔️ |
$current_major_version = 15.0
NOTE
: The $deprecated_functions_versions
array was amended to include 15.0
$initial_version | bp_get_deprecated_functions_versions() | passed |
---|---|---|
15.0 | 0.0 | ❌ |
14.0 | 12.0, 15.0 | ❌ |
12.0 | 12.0, 14.0, 15.0 | ✔️ |
11.0 | 12.0, 14.0, 15.0 | ✔️ |
0 | 14.0, 15.0 | ❌ |
Given the previous results, I decided to simplify the bp_get_deprecated_functions_versions()
function by adding a filter hook bypass_bp_source_subdirectory_check
so that a development version can replicate the loading of deprecated functions the same as non-development versions, e.g., production, and simplified the code for determining which deprecated functions to include. The following are the test results for the "Proposed New Patch" (see attached proposed-new-9210.01.patch)
With Proposed New Patch
$current_major_version = 14.0
$initial_version | bp_get_deprecated_functions_versions() | passed |
---|---|---|
14.0 | 12.0 | ✔️ |
12.0 | 12.0, 14.0 | ✔️ |
11.0 | 12.0, 14.0 | ✔️ |
0 | 12.0, 14.0 | ✔️ |
$current_major_version = 15.0
NOTE
: The $deprecated_functions_versions
array was amended to include 15.0
$initial_version | bp_get_deprecated_functions_versions() | passed |
---|---|---|
15.0 | 12.0 | ✔️ |
14.0 | 12.0, 14.0, 15.0 | ✔️ |
12.0 | 12.0, 14.0, 15.0 | ✔️ |
11.0 | 12.0, 14.0, 15.0 | ✔️ |
0 | 12.0, 14.0, 15.0 | ✔️ |
$current_major_version = 16.0
NOTE
: The $deprecated_functions_versions
array was amended to include 16.0
$initial_version | bp_get_deprecated_functions_versions() | passed |
---|---|---|
16.0 | 0.0 | ✔️ |
15.0 | 15.0, 16.0 | ✔️ |
14.0 | 15.0, 16.0 | ✔️ |
12.0 | 15.0, 16.0 | ✔️ |
11.0 | 15.0, 16.0 | ✔️ |
0 | 15.0, 16.0 | ✔️ |
In conclusion, I don't rely on unit tests for a variety of reasons, just to name a few:
- Do they run to verify the intended change works as intended?
- If they run do they provide enough coverage to detect faults?
I don't know the answers to these questions because, I'm not set-up to create, run nor verify the accuracy of unit tests.
This brings up testing in general at the ticket level, e.g., unit tests (if applicable), manual tests, etc., we don't include a test plan, typically a detailed test report (based off of the test plan) is not included and we don't have documentation that outlines/details the ticket process.
#10
@
6 months ago
Hi @emaralive
Thanks a lot for your amazing work on this. In short I've updated the PR according to your patch.
The only thing I've edited is this check if ( (float) 15 >= $current_major_version )
, because we need to test against the first installed version to match what we discussed about during development chat. Carry on loading 12.0 deprecated functions until the first install version is > 15.0.
I've split the unit tests in two functions:
test_bp_get_deprecated_functions_versions_array_is_consistent_with_deprecated_files()
is there to check that when we create a new/src/bp-core/deprecated/X.X.php
file we don't forget to update the$deprecated_functions_versions
to includeX.X
version.test_bp_get_deprecated_functions_versions()
is checking we include the last 2 deprecated function files or all deprecated functions from 12.0 if the first installed version is <= 15.0.
About:
there should be a documented "Deprecation Policy"
I totally agree: https://github.com/buddypress/bp-documentation/issues/290
Can you confirm the updated PR is fine with you? If so I'll commit it.
@emaralive commented on PR #352:
6 months ago
#12
I see that the filter hook from the proposed-new-9210.01.patch was dropped with no explanation as to why. The way I see it, as it stands, with a development version the only choices for loading deprecated functions is to:
- Don't load any deprecated functions.
- Load all deprecated functions.
Which means reliance on the unit tests to catch issues, and in this case, seem to have been unreliable. Which is why I added the hook to allow for a 3rd option of allowing deprecated functions for development version to load the same as a production version which represented a manual fail safe check. Perhaps, someone could explain.
6 months ago
#13
Sorry @emaralive I thought this part of my comment on the Trac ticket was explaining the scandir that is only done during unit tests:
test_bp_get_deprecated_functions_versions_array_is_consistent_with_deprecated_files()
is there to check that when we create a new/src/bp-core/deprecated/X.X.php
file we don't forget to update the$deprecated_functions_versions
to includeX.X
version.
Deprecated code is not not loaded into this function but here: https://github.com/buddypress/buddypress/blob/8cfca1802adeeb85fff69b8848deec6996d49e39/src/class-buddypress.php#L652
6 months ago
#14
I see that the filter hook from the proposed-new-9210.01.patch was dropped with no explanation as to why. The way I see it, as it stands, with a development version the only choices for loading deprecated functions is to:
- Don't load any deprecated functions.
- Load all deprecated functions.
Which means reliance on the unit tests to catch issues, and in this case, seem to have been unreliable. Which is why I added the hook to allow for a 3rd option of allowing deprecated functions for development version to load the same as a production version which represented a manual fail safe check. Perhaps, someone could explain.
Hi @emaralive sorry it took me a while to understand your point, here's the explanation and I'm open to add a filter if needed:
https://buddypress.trac.wordpress.org/ticket/9224
See #358
#16
@
6 months ago
- Owner set to imath
- Resolution set to fixed
- Status changed from new to closed
In 13999:
#17
@
6 months ago
- Keywords needs-dev-note needs-docs added; has-patch has-unit-tests has-testing-info removed
- Milestone changed from 14.1.0 to 15.0.0
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening this ticket and moving it into the 15.0 milestone so that we think about:
- writing a 15.0 developer note: this will be the last major version to include 12.0 deprecated code,
- adding an Admin Notification or an Administrator notice once #9098 is achieved,
- writing the deprecation policy.
In 13964: