Closed jamesozzie closed 3 years ago
cole10up
commented
4 years ago @jamesozzie - Can you give this another try on your side? We've since released a hotfix and I see a new version of WP User Frontend. I have both new versions and unable to reproduce this.
Plugins and versions:
SK Dashboard and page speed looking just fine.
jamesozzie
commented
4 years ago @cole10up LGTM. I tested with WP User Frontend 3.1.17 & 3.1.18. No issues from my side.
kabirbd89
commented
4 years ago But, I am still getting problem on my site. WP User Fronted 3.1.18 and Site kit 1.5.1.
kabirbd89
commented
4 years ago Well got it, I tried it another website with fresh new install of both plugin and it was completely okey until I create a post form in WP User Fronted, Can you check again with a form. @jamesozzie
jamesozzie
commented
4 years ago @cole10up @kabirbd89 From testing today this same issue remains (without having to use a form).
I'll check other sites also
jamesozzie
commented
4 years ago kabirbd89
commented
4 years ago @jamesozzie I am thinking about a temporary solution in completely non professional way. Which is 302 redirection. does google mark it negatively ! any idea
jamesozzie
commented
4 years ago @kabirbd89 Apologies for the late response. That sounds like a question for the Webmaster Community experts I'm afraid. But rest assured we investigate this further.
jamesozzie
commented
4 years ago @aaemnnosttv Assigning this back while I take another look.
jamesozzie
commented
4 years ago @aaemnnosttv Performed some more testing on this today, no console errors and all 200 responses. Same issue persists and easily reproducible. I'll unassign now if you'd like to pick it up once more, additional details below.
We’ve assembled some links to get you started:
\n\t\n\t\t\t\tor, change your theme completely\t\t\t
\n\t\t\t![](\"https://api.cm-wp.com/wp-content/uploads/2019/11/instagram_horizont.jpg\")
\n\t\t\t\t\t\t\tYour site has critical issues that should be addressed as soon as possible to improve the performance or security of your website.\t\t\t\t\t
\n\t\t\n\t\t\t\t\n\t\t\tTake a look at the 7 items on the site health check status page.\t\t
\n\t\t\n\t\tWordPress 5.4.1 running generatepress child theme theme.
\tNo activity yet!
Session expired
\n\t\tPlease log in again.\n\t\tThe login page will open in a new tab. After logging in you can close it and return to this page.
\n\t
eugene-manuilov
commented
3 years ago @felixarntz @aaemnnosttv
Ok, i can confirm the issue is on our end and it happens because we select a random post when we try to get a reference entity and there is no such. In this particular case it gets a random post which is of wpuf_forms type and that post type is hidden (public = false). That's why lighthouse sees 404 response when it tries to examine public page of a hidden entity.
Basically, what we need to do here is to update the line number 201 here: https://github.com/google/site-kit-wp/blob/0413b5fcc00e8773075edf9b4032487ee71e982f/includes/Context.php#L192-L206
Instead of getting a random post using get_post() function, we need to check if there is a queried post first. If it exists, then it's safe to call get_post() function. If not, then we need to call get_posts( 'numberposts=1' ) instead and take the first post in the array if it is not empty. Finally, if get_posts returns an empty array, we need to return null and let PSI check homepage performance.
PS: do we really need to get a random post there? This logic seems strange to me. IMO we need to fallback to the default value which is the homepage if there is no queried post. Let me know if I am missing something.
aaemnnosttv
commented
3 years ago Thanks for the explanation @eugene-manuilov ! Moving this forward to be prioritized.
@felixarntz should we rename this issue since it doesn't seem to be specific to the frontend plugin?
felixarntz
commented
3 years ago @eugene-manuilov I think the problem here is not so much that we get a "random" post, but that the post may not be public. get_post() returns the current post currently being edited in the admin (if there is any), so that's all good I think. What we need to add here is checks get_post_type_object( $post->post_type )->public && ( $post->post_status === 'publish' || $post->post_status == 'inherit' && $post->post_type === 'attachment' ) - only if these are fulfilled, this is an entity for Site Kit purposes (every entity should have a public URL).
Regardless of that, is this maybe a problem with the WP User Frontend plugin too? That plugin shouldn't be setting the $post global on the Site Kit dashboard I'd say.
eugene-manuilov
commented
3 years ago @felixarntz get_post() returns the current post but it doesn't guaranty that it's currently being edited in the admin. IMO we need to change how we get the current post because currently we use unreliable approach that can be broken by other plugins too.
felixarntz
commented
3 years ago @eugene-manuilov Maybe we could wrap it into a clause which checks that the get_current_screen() is the post editing screen? That would make it a bit more bullet-proof.
However I'd argue even the current logic is correct - if a plugin sets the $post global somewhere that it affects the Site Kit dashboard, that plugin is clearly doing it wrong. But I agree with your concern, we only care for the current $post when editing a post in the admin, so let's add a check around it to only use that global when on the post edit screen.
Regardless of that though, the checks for that post being publicly available should also be added.
eugene-manuilov
commented
3 years ago Yes, I agree with you, adding get_current_screen and post availability checks is what we need regardless of what other plugins do.
felixarntz
commented
3 years ago @aaemnnosttv IB mostly ✅ , one additional thing to add:
![\"\"](\"https://httpster.co/wp-content/plugins/google-site-kit/dist/assets/images/logo-g_white_small.png\"){kind=logo}
![\"\"](\"https://httpster.co/wp-content/plugins/printful-shipping-for-woocommerce/assets/images/printful-menu-icon.png\"){kind=icon}
Context::get_reference_entity_from_url, since that bit is just as relevant there. Probably worth putting into a private method to call from both places.cole10up
commented
3 years ago Tested
Installed latest SK, activated and completed setup. Activated Analytics modules.
Downloaded WP Frontend here https://wordpress.org/plugins/wp-user-frontend/
Activated WP Frontend and sipped setup
Navigated to Post form in WP Frontend
Added a blank form:
Saved:
Navigated to SK Dashboard and checked SK entity data:
Navigated to post with data and checked entity data:
Navigated to post URL from 'More details' admin bar link
Checked entity data:
Noticed Entity data is showing forward and backward slashes in the URL section. Is this expected @aaemnnosttv @eugene-manuilov?
Sending to CR to confirm the above ^^
felixarntz
commented
3 years ago @cole10up The backslashes there are correct, they're only in the encoded JSON object.
cole10up
commented
3 years ago Looks good on my end per Felix's comment above.
kabirbd89
commented
3 years ago everything okay except the post admin bar. The SK menu does not appear in the admin bar for post pages. is it intentional!
Bug Description
Site Kit main dashboard don't work as expected with WP User Frontend activated, with the following issues:
Individual dashboards work fine
Site Kit version version 1.5.0 WP User Frontend version 3.1.17
First reported in WordPress support form, and reproducible from support:
Issue also reported on WP User Frontend support forum: https://wordpress.org/support/topic/conflict-with-site-kit-by-google/
Additional information:
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
WP_Postobjects as entities where both the post type ispublicas well as the post itself has been published already (post status ispublish, orinheritin the case of anattachment).Implementation Brief
Update logic in
Context::get_reference_entitywhenis_adminwith aWP_Postinstance (aftergooglesitekit-dashboardcase)WP_Screen, or itsbaseis notpostand itsparent_baseis notedit, returnnullThis ensures that the subsequent detection is only done for the post edit screen (block editor or classic)
public, returnnullpublishreturnnullattachment, in which case the post's status should beinheritget_post_statusshould be used here as it handles the status inheritance for attachmentsContext::get_reference_entityQA Brief
User Frontend > Post Forms > Add Form - use template Post form and click "Create form", then click on "Save form" (no additional input needed)
_googlesitekitEntityDatain the console, all values should benull_googlesitekitEntityDatain the console, all values should match the current post being edited_googlesitekitEntityDatain the console, all values should match the post that was navigated fromChangelog entry