boonebgorges / buddypress-docs

GNU General Public License v3.0
106 stars 44 forks source link

BP 12.0: Load order makes certain info unavailable when loading group extension #739

Closed boonebgorges closed 6 months ago

boonebgorges commented 7 months ago

Load order changes in BP 12.0 mean that certain information about the "current group" is no longer available during the BP_Docs_Group_Extension constructor method. This results in certain settings not being loaded properly, and also breaks access checks. See https://wordpress.org/support/topic/menu-item-documents-not-visible-for-simple-group-members/#post-17273517

The solution should be to move permission checks and the loading of settings into methods that are called later in the bootstrap process. See https://github.com/dcavins/buddypress-docs/commit/4256d1c1f9f7f90763612c42270ca65ca48de1c6 for one possible implementation.

boonebgorges commented 7 months ago

@dcavins Taking this issue out of our email so we don't lose track :)

Behu38 commented 7 months ago

Hi Boone, It's Bertrand (@behu6338), I opened the initial topic about the issue, on the WordPress forum. Thanks for the following. So, I have to see with the code given here ? To be sure, the file to modify is : in the buddypress-docs plugin folder > includes/integration-groups.php ? My installation use a child theme ; do I have to implement it in the child theme ? Thanks

boonebgorges commented 7 months ago

Hi @Behu38 - Thanks for joining the thread. If you feel confident doing so, you may replace the entire contents of your buddypress-docs/includes/integration-groups.php file with what you see here: https://raw.githubusercontent.com/dcavins/buddypress-docs/4256d1c1f9f7f90763612c42270ca65ca48de1c6/includes/integration-groups.php

If you are not comfortable doing so, and you don't have the necessary skills to debug if something goes wrong, I strongly recommend waiting until we're able to push out a release.

Behu38 commented 7 months ago

Hi @boonebgorges Thank you for your reply ! Yes, I think I will wait until your next release. There's no emergency to fix that issue, and I prefer not crash the client website just before Christmas :) Have you an idea when the release will be push out ? Thanks,

Regards

boonebgorges commented 7 months ago

Thanks @Behu38 . We'll probably push out an update today or tomorrow, but I leave it to your discretion whether to crash your client's site before Christmas :)

@dcavins If you're happy with your fix, I'll leave it to you to merge into the 2.2.x branch. I can test it after you've merged if you'd like.

dcavins commented 7 months ago

@boonebgorges Thanks! I have merged it in and would appreciate it if you could test the changes.

boonebgorges commented 7 months ago

Thanks, @dcavins . I found some more cases where the necessary data couldn't be loaded, mostly during group edit/create. I've done some additional cleanup in 85b8851.

dcavins commented 7 months ago

I'm a little concerned that $this->group_id isn't set in all of those cases in the parent class. I'll have to take another look at the updates to the BP_Group_Extension to make sure that it's catching the creation/edit cases. Thanks!

boonebgorges commented 7 months ago

This part of the Docs codebase goes way, way back, before a bunch of the BP_Group_Extension rewrite stuff that happened in BP 1.6, 1.7ish? So it intentionally doesn't try to rely on the information provided by the parent class. But if you think you can safely make the changes, go ahead and do so.

boonebgorges commented 6 months ago

@dcavins How are we feeling here sir?

dcavins commented 6 months ago

I think we are good! This change tested well for me. I would like to later take a longer look at the group extension base, but I think we're good to close this ticket. Thanks!

boonebgorges commented 6 months ago

Rock on.