Review the way we load 12.0.0 deprecated code

[imath]

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.

[imath]

In 13964:

Load 12.0.0 deprecated code when upgrading from 12.0.0

See #9210 (trunk)

[imath]

In 13965:

Load 12.0.0 deprecated code when upgrading from 12.0.0

See #9210 (branch 14.0)

[emaralive]

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.

[imath]

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

Always Include 12.0 deprecated functions as soon as initial version is < 15.0.0.

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/9210

[imath]

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

[emaralive]

@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:

1. Activate plugin
2. Switch/login to a non-administrator account, e.g., subscriber
3. 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:

1. Activate plugin
2. As adminstrator visit site.url/wp-admin/options-general.php?page=bp-simple-events
3. Select a non-administrator user role that coincides with an active user with same assigned user role, e.g., subscriber, and then "Save Roles"
4. 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
5. 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:

1. Activate plugin - default settings will suffice
2. If no groups are created, enable groups component, create a group and enable forums for group.
3. 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.
4. 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).

[emaralive]

@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.

[emaralive]

Proposed New Patch

[imath]

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 include X.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.

[espellcaste]

I left a few comments in the pr. And the idea of having a doc is great, btw. o/

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:

1. Don't load any deprecated functions.
2. 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.

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 include X.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

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:

1. Don't load any deprecated functions.
2. 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

[imath]

In 13998:

Make sure 12.0 deprecated code is loaded until version 15.0

Version 12.0 changed BuddyPress a lot with the introduction of the BP Rewrites API. To give more time to advanced users, BP plugin or theme developers so that they make their code ready for this API, unlike how we deal with deprecated code since r13304 & for the 2 following major releases (14.0 & 15.0), we will carry on loading it to prevent potential errors.

Props emaralive, espellcaste.

See #9210 (trunk)
Closes ​https://github.com/buddypress/buddypress/pull/352

[imath]

In 13999:

Make sure 12.0 deprecated code is loaded until version 15.0

Version 12.0 changed BuddyPress a lot with the introduction of the BP Rewrites API. To give more time to advanced users, BP plugin or theme developers so that they make their code ready for this API, unlike how we deal with deprecated code since r13304 & for the 2 following major releases (14.0 & 15.0), we will carry on loading it to prevent potential errors.

Props emaralive, espellcaste.

Fixes #9210 (branch 14.0)

[imath]

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.

[imath]

In 14013:

Introduce a function to inform about whether BP was loaded from src

bp_is_running_from_src_subdirectory() informs whether BuddyPress was loaded from the src subdirectory (trunk version). This function exposes the 'bp_is_running_from_src_subdirectory' to let developers tests code that is only available when BuddyPress is built.

Props emaralive, espellcaste.

See #9210
Fixes #9224
Closes ​https://github.com/buddypress/buddypress/pull/358