Why put this selector in the widgets datastore? It's related to a user's "User Input" responses, and isn't specific to widgets.

@tofumatt the AC calls for it, does it not? Ok to revise that if it makes sense but it seems to be consistent with what the AC calls for. Also I think "metrics" in this sense is synonymous with "widgets". Answer-based metrics are the widgets that correspond to a user's answers to the user input survey. Is that correct?

Thanks for reporting, I need to revisit this as there are probably a number of maintenance updates that need to be made 👍

Thanks @marrrmarrr! Moving this over into IB 👍

IB ✅

Thanks @techanvil, that sounds reasonable to me it just reads a bit verbose. How about googleTagLastSyncedAtMs? 😄

I'll make this change to avoid another iteration here as the rest of it LGTM 👍

@felixarntz I've updated the AC here, which should cover everything now. Once this looks good to you I will add the IB.

A few final points/questions for you:

• The endpoint and selector for getting the survey now has a singular name to better reflect that only a single survey may be returned
• Question: the current survey timeouts we have are survey-specific and the TTL comes from the trigger. Do we want to preserve survey-specific timeouts in addition to a "global" survey timeout of 12 hours? I don't think that would be necessary anymore since we're serving surveys from a queue so users should only see the next one as early as 12 hours later. It sounds like this could be simplified a fair bit into a single timeout that is specific to survey display only. Does that sound right to you?

• • We would also need another endpoint like mark-survey-shown that removes a survey from the queue as it is being shown in the UI and resets the persistent timeout for showing any surveys from the point above.

  • Could we not use the existing survey_shown event for this?

Let me know if anything else seems to be missing or needs clarification 👍

Ah thanks for clarifying, that makes sense. Can you please clarify that in the ACs (that it should happen in the trigger endpoint)?

Will do 👍

Would the trigger endpoint then still return the survey, or should we change it to just return some kind of {success:true} response? That may be better, to clarify the use-case, since any returned survey should not be displayed anyway.

Yes, a minimal response would suffice; SGTM. Aside from a status, we could maybe also return a value as to whether a survey was queued or not but we can keep that in mind if/when it's needed.

I think we only have a single place to display surveys (bottom right corner of the screen), and we would never want to show 2 right after each other, so I think changing the endpoint to always return a single survey or null makes most sense.

Sure, I was more thinking of the screen, but if we're only ever going to surface surveys on the main dashboard then this wouldn't be a problem 👍

the only thing I would suggest to clarify is to point out a concrete REST endpoint to implement for adding a survey to the queue

@felixarntz I'm not quite sure what you mean. I was thinking the survey would be added to the queue as part of the backend handling for the trigger endpoint since those will continue to happen on-demand as they do now, as the response on the client side won't really be used/needed anymore.

I also wonder whether we really need the endpoint to return surveys from the queue, shouldn't it rather be an endpoint to return a single survey to display or null?

I suppose it could be only a single survey if we limit surveys to only be shown in one place/context. Otherwise, I see them potentially being similar to feature tours where multiple could exist but be context-specific. Apart from that, we could make it specific to returning a single survey or none.

Other than that, we should indeed also cover the logic about displaying surveys. How about the following

Sounds good to me, I'll iterate on the AC to include this 👍

IB ✅

One solution here would be to include the symfony/polyfill-mbstring package (v1.19 supports down to PHP 5.3) although if we can replace this with one of our other methods that don't rely on this, that may be better.

Bug Description

In #5868 we implemented a fix which resolves redirect issues on sites using internationalized domain names (IDN). This involves the use of a core PHP multibyte string function which may not exist on some environments. While the mbstring module is commonly enabled, it is not enabled by default so we cannot rely on it to be generally available.

WordPress polyfills a few mb_* functions, (e.g. mb_substr, and mb_strlen) but not all of them.

Steps to reproduce

Various actions could raise this error as the hooked function runs during wp_validate_redirect which itself is called by various functions, notably wp_safe_redirect()

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• Site Kit should not raise fatal errors in environments where the mbstring PHP module is not enabled
  • More specifically, Site Kit should not rely on any mb_* function to be provided by PHP

Implementation Brief

Test Coverage

QA Brief

Changelog entry

Regarding the last idea there, I just created a quick POC branch which seems to work pretty well. You can see it here https://github.com/google/site-kit-wp/compare/poc/setting-on-update

Create Module_With_Data_Available_State_Trait trait in Google\Site_Kit\Core\Modules namespace.

• Add a protected $transients instance variable.
• Add an abstract setup_transients method that returns Transients instance.
• Add a protected get_transients method:

After giving this some more thought, I think the simplest solution is to add a Transients instance on the base Module class. They already have Options and User_Options so this fits into what a module has access to. This could be instantiated in the constructor given the module's Context instance.

The Site_Verification module already has a few uses of transients inline, so these could be refactored at the same time.

With this added, the trait can assume transients are available like any other property on Module. This should simplify things a fair bit I think.

• Add a protected get_transient_slug method

This method name sounds generic but the usage here is specific to the transient for the data-available state. Let's name it more specifically to what it's for, perhaps get_data_available_transient_name? (internally it's referred to as a name rather than a slug)

• Add a update_option_googlesitekit_{search-console/analytics/analytics-4}_settings action to compare the current and new search console property/ga3 property/ga4 measurement id and call reset_data_available method if they are different.

I'd like to see this implemented in a way that doesn't duplicate this across modules. This might even justify a prerequisite issue, but we do a similar thing in a few places. I'm thinking we could add a new on_update( callable ) instance method to the Setting class. This would be a wrapper for a few add_action calls, similar to Setting_With_Owned_Keys_Trait::register_owned_keys in that it would hook on update_option_{$option} and add_option_{$option}" (since WP treats these as separate). Then in the Module_With_Data_Available_State_Trait we could have a method to register_data_available_reset which could take the keys to "observe" in the listener callback as an argument. These could probably even be provided by $this->get_owned_keys() in this case, but we might want to be more specific about it and only pass the specific keys in the AC. Happy to discuss this part more or split out a separate issue for the infra here but let me know what you think.

Thanks @kuasha420 – apologies for the delay here – this is really close, the only thing I think there is to finalize here is around the internal use of transients (methods, etc). I have a few ideas that I'll follow up with here shortly.

Looks like we need to add 8.2 to the test matrix. Feel free to open a PR for that if you'd like?

Thanks @techanvil ! IB overall reads great to me, just one last question.

One thing I'm thinking about is the format of the googleTagLastSyncedAt value in case we ever wanted to use this in PHP too, since PHP references the epoch in seconds where as Date.now is in milliseconds. We could potentially convert the integer into seconds when saving, and back to millis when passing to the client, or we could use a datetime string which might be less vulnerable to mistakes in translation or errors in interpretation. What do you think?

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• Release is prepared, tested, and deployed to wordpress.org.

Implementation Brief

• N/A

QA Brief

• N/A

Changelog entry

• N/A

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• Release is prepared, tested, and deployed to wordpress.org.

Implementation Brief

• N/A

QA Brief

• N/A

Changelog entry

• N/A