Skip to content

Conversation

ideag
Copy link
Contributor

@ideag ideag commented Jun 7, 2025

rework dotorg_connectivity site health check to not reference .org specifically

…ecifically fairpm#9

Signed-off-by: Arūnas Liuiza <ask@arunas.co>
@Ipstenu
Copy link
Contributor

Ipstenu commented Jun 8, 2025

I was afraid this would result in having to rebuild the JS. Thank you for tackling this @ideag !

@ideag
Copy link
Contributor Author

ideag commented Jun 8, 2025

Happy to help!
I'm still not sure what to do with the dotorg url in the actions part. It's so generic it seems better to just remove it.
I also hate how I have to regex the error message in, but I see no way around that at the moment.

Signed-off-by: Arūnas Liuiza <ask@arunas.co>
@ideag
Copy link
Contributor Author

ideag commented Jun 9, 2025

I've updated the regex to support translations as well. Decided to leave the actions link to dotorg as is for now. We can revisit if/when we have a different link to use there.

@ideag ideag marked this pull request as ready for review June 9, 2025 12:52
@Ipstenu Ipstenu linked an issue Jun 9, 2025 that may be closed by this pull request
@circuitaxel
Copy link

Great work discovering the JS approach!

I wonder if we could simplify this, by leveraging the rest_post_dispatch filter? That would allow us to combine all the logic in a single PHP filter.

Optionally, filtering the site_status_tests and removing the one for ['async']['dotorg_communication'] and replacing it with our own, but then we need to also create the rest lookup and such. It depends entirely on how much control we want over the entire lifecycle of the test.

@ideag
Copy link
Contributor Author

ideag commented Jun 13, 2025

I thought about that, but using a purpose-specific filter seemed cleaner to me. Moving this to rest_post_dispatch also does not solve the need to parse the API response with regex, so it doesn't seem to be an improvement in that respect.
Converting this to a non-async check is an option, but a more involved one. I assume dotorg did it this way with performance in mind and I agree with that judgement.

@circuitaxel
Copy link

Filtering the site_status_test doesn't restrict you to only doing synchronous tests, it can be added as a new async test, it does provide the most control, and retains the performance intent behind the original implementation, but does mean we need to write the whole response our selves (but that may be preferable to just conditionally replacing the strings of a response?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure that Site Health is not pinging .org
3 participants