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

Add preliminary support for json metadata client discovery #268

Merged
merged 4 commits into from
Jul 7, 2024

Conversation

dshanske
Copy link
Member

#267 per request, basic support for the client ID being a JSON file using the properties identified.

@aaronpk This should work based on the notes.

If this gets adopted into the spec, I'll probably cut the unsupported manifest side file code, as that proposal never took off, and simplify for JSON with a HTML MF2 fallback.

@dshanske
Copy link
Member Author

@pfefferle Can you have a look?

@janboddez
Copy link
Contributor

Ha, missed this PR the other day. I'll see if I can try it tonight!

@dshanske
Copy link
Member Author

Feedback is appreciated

libxml_use_internal_errors( true );
if ( function_exists( 'mb_convert_encoding' ) ) {
$content = mb_convert_encoding( $content, 'HTML-ENTITIES', mb_detect_encoding( $content ) );
$content_type = wp_remote_retrieve_header( $response, 'content-type' );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parse() seems like it should be designated as static since that's how it is used. Also it seems as though it could get to the bottom and not return so I think it needs a return at the bottom. I also suggest a doc block describe the params and what it does if there's an error.

$content = mb_convert_encoding( $content, 'HTML-ENTITIES', mb_detect_encoding( $content ) );
$content_type = wp_remote_retrieve_header( $response, 'content-type' );
if ( 'application/json' === $content_type ) {
$json = json_decode( wp_remote_retrieve_body( $response ), true );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$json = json_decode( wp_remote_retrieve_body( $response ), true );
/** @var $json array{
* client_id: string,
* client_name: string,
* logo_uri: string
* }
*/
$json = json_decode( wp_remote_retrieve_body( $response ), true );

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like using comments to describe what we expect to be in json like this since it makes identifying and using them slightly easier.

@janboddez
Copy link
Contributor

janboddez commented Jun 28, 2024

So, the PR works!

Also (I can't add a comment because it isn't new), this here:

private function get_html( $input ) {
	if ( ! $input ) {
		return;
	}

$input is always an instance of DOMDocument (or Masterminds\\HTML5), so you will never return here. Not really a problem, but it doesn't really add anything. I think.

Also, the issue I had earlier was caused because of:

$this->html['title'] = $xpath->query( '//title' )->item( 0 )->textContent;

If the response contains no title (rare, for HTML documents, but not impossible), you'd get at least a warning here. (Though somehow I was getting an infinite loop.) Better assign the outcome of $xpath->query( '//title' ) to a variable and then loop over it (and break after the first iteration). Or run it through if ( ! empty() ) or similar.

Not sure what happens if no meaningful client details are discovered? I liked Aaron's suggestion (I think) of just showing a URL ...?

@dshanske dshanske removed the request for review from aaronpk June 29, 2024 14:54
@dshanske dshanske merged commit f19ddb1 into indieweb:trunk Jul 7, 2024
6 checks passed
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.

3 participants