Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sorry, you are not allowed to access this page when activating Social Meta module #1066

Closed
wpsmort opened this issue Aug 9, 2017 · 13 comments

Comments

Projects
None yet
4 participants
@wpsmort
Copy link
Member

commented Aug 9, 2017

When activating the Social Meta module, the first time you click on the Social Meta menu item you get a Sorry, you are not allowed to access this page error.

This is because the menu link is http://testsite.dev/wp-admin/admin.php?page=all-in-one-seo-pack/modules/aioseop_opengraph.php when it should be http://testsite.dev/wp-admin/admin.php?page=aiosp_opengraph

Need to find when this broke.

@wpsmort wpsmort added the Bug label Aug 9, 2017

@wpsmort wpsmort added this to the 2.3.16 milestone Aug 9, 2017

@wpsmort wpsmort self-assigned this Aug 9, 2017

@michaeltorbert michaeltorbert assigned arnaudbroes and unassigned wpsmort Aug 9, 2017

@arnaudbroes

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2017

This started with 2.3.15
So apart from this issue, the weird thing is that the Social Meta module file "aioseo_opengraph.php" is located in "/all-in-one-seo-pack/modules/aioseo_opengraph.php".

When a page is added to the submenu, the menu item refers to "admin.php?page=all-in-one-seo-pack/modules/aioseop_opengraph.php" while the working slug is "admin.php?page=aiosp_opengraph".

A few other menu items refer to "/modules/%module_name%.php" while some others refer to "%module_name%.php", even though all of them are located in /all-in-one-seo-pack/modules/...
So apart from this issue, I think we should take a look later on how this is happening.

@michaeltorbert

This comment has been minimized.

Copy link
Member

commented Aug 9, 2017

In aioseop_module_class.php function add_menu() there are three add_submenu_page()s.

The first one, on line 1971 is how most modules get added.
The 2nd one (default) is for General Settings.
The third one is how Social Meta and Robots.txt get added. The difference the 6th parameter, $this->get_prefix( $k ) . $k vs plugin_basename( $this->file )
Subbing in plugin_basename( $this->file ) works. We need to figure out if this was done on purpose or not.

@wpsmort

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2017

@michaeltorbert I tracked this bug back to this commit - 98703b5

@wpsmort wpsmort assigned michaeltorbert and unassigned arnaudbroes Aug 9, 2017

@michaeltorbert

This comment has been minimized.

Copy link
Member

commented Aug 10, 2017

I'm not sure why that PR tagged this issue, but it's unrelated and can be ignored within the context of this issue.

@michaeltorbert

This comment has been minimized.

Copy link
Member

commented Aug 10, 2017

The problem is the settings being moved to a new init() function on the WP init action hook. This has the information needed to add the submenu, which needs to be available before the init function runs.
@amostajo Do you have any idea why this was moved?

@amostajo

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2017

@michaeltorbert This was moved under "init" hook because the module was being initialized before wordpress or external plugins were initialized/instantiated, this was causing an scope issue, because the module was limited to default post types and taxonomies. Custom types/taxonomies registered by other plugins were left out of the scope.

Can't recall if this was related when doing the taxonomies task, but custom things registered by 3rd party plugins where not showing in the checkboxes because the menu/submenu was being genereated before wordpress initializes itself and all plugins.

@michaeltorbert

This comment has been minimized.

Copy link
Member

commented Aug 11, 2017

@amostajo What are your thoughts on how we should handle this?

  1. Swapping $this->get_prefix( $k ) . $k with $k vs plugin_basename( $this->file ) as I described above fixes it, but I'm not sure what else that may break.

  2. Completely rewrite how the menus are created. (Long overdue, but that may be overkill just to fix this).

  3. Move the code back to the constructor from init(). (What would this break? Is there another way to fix those other than by moving this code? If this stuff is running to early, keep in mind that the info is stored in a global variable.)

  4. Simply add $this->init() to the constructor. (Not sure if this breaks anything, but it seems kind of an ugly way of doing it. Surely there's a better way.)

@amostajo

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2017

@michaeltorbert The right thing to do would be option (2), imo. Because menus should be created following Wordpress initialization flow. Menus should not be created before Wordpress initializes itself and plugins.

My comments to the other options:

Swapping $this->get_prefix( $k ) . $k with $k vs plugin_basename( $this->file ) as I described above fixes it, but I'm not sure what else that may break.

No sure... without unit testing this is unknown... and every little change in this plugin always affect something, my past experience...

Move the code back to the constructor from init(). (What would this break? Is there another way to fix those other than by moving this code? If this stuff is running to early, keep in mind that the info is stored in a global variable.)

The issue reported were custom post types/taxonomies are not showing will reappear. Cuz it is initializing menus before wordpress init.

Simply add $this->init() to the constructor. (Not sure if this breaks anything, but it seems kind of an ugly way of doing it. Surely there's a better way.)

Same issue as below. The only difference is that part of the code is on a method rather than constructor.

@michaeltorbert

This comment has been minimized.

Copy link
Member

commented Aug 11, 2017

I agree 2 is the best solution, at least long term, but it may take quite a bit to do it. I've never liked how complicated the menu code is. It's normally pretty simple in WP plugins. We should probably first figure out why the behavior of 1 exists in the first place though before we ditch it completely.

For 4, I meant that we call init() on the init action, as well as from the constructor (as a temporary solution). I'm not sure if that causes any problems.

@amostajo

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2017

Will test it

amostajo added a commit to amostajo/all-in-one-seo-pack that referenced this issue Aug 12, 2017

semperfiwebdesign#1066 Force init on opengraph
Calls init outside Wordpress initialization flow to prevent plugin menu
issues.
@amostajo

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2017

Did some testing and appears to work fine without any errors thrown.
@wpsmort This might need deep testing though not sure what may affect, would you like a PR for pro as well?

@michaeltorbert

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

@amostajo Is there a PR for this?
Since this is pretty simple code-wise, just a PR in free will probably suffice, and for Pro testing @wpsmort can just copy/paste the line of code. Testing needs to be done in free and Pro... it's unclear what, if anything, this may affect.

@wpsmort

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2017

PR #1082 has been tested and is good.

@wpsmort wpsmort assigned michaeltorbert and unassigned amostajo Aug 17, 2017

michaeltorbert added a commit that referenced this issue Aug 17, 2017

#1066 Force init on opengraph (#1082)
Calls init outside Wordpress initialization flow to prevent plugin menu
issues.

@michaeltorbert michaeltorbert removed their assignment Aug 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.