AMP schema refactor #509
AMP schema refactor #509
Conversation
Work with the plain array data for as long as possible, and then recursively convert at the end.
Can't have publisher type without a name, and can't have a name without a type, so consolidate into a single conditional Also checks if the publisher even exists before adding what could otherwise be empty values.
If the site icon (in Customizer) is populated, then it is passed through from AMP and added to liveblog entry schema data. See Automattic#506.
This is a recommended property for BlogPosting. See Automattic#506.
Just use the AMP-supplied publisher data as is, if it exists (which it should, to be valid in AMP).
There was a problem hiding this comment.
Thanks for this Gary, this looks good - I especially like how good the commit messages are! I've left one inline comment about some code I'm unsure of.
Also a question - what can we do here to make sure that this code produces expected results?
Oh, and is your intention to develop this further to abstract this out for use in both AMP and non-AMP liveblogs?
classes/class-wpcom-liveblog-amp.php
Outdated
| } | ||
|
|
||
| $metadata['@type'] = 'LiveBlogPosting'; | ||
| $metadata['liveBlogUpdate'] = $blog_updates; | ||
| $blog_updates[] = json_decode( json_encode( $blog_item ) ); |
There was a problem hiding this comment.
Can you elaborate on the need for this change please? I'm not sure I understand what it adds.
There was a problem hiding this comment.
The previous code was casting each array to an (object), which makes adding and amending data in the array (such as a conditional publisher logo) awkward. This conversion should be a one-time recursive task. json_decode( json_encode() ) is a short-hand way of doing just that. This could be pulled into a separate method (such as convertToObjects(), or a better domain-related name) once the abstraction was done.
…eblogs Moves the actual generation of the Liveblog metadata into it's own helper function in the main WPCOM_Liveblog class and then uses that function to output the metadata in both the AMP and non-AMP versions of a Liveblog.
Also make sure it's actually used!
|
I decided to tackle the abstraction myself and opened GaryJones#1 |
Unit and integration tests would be the obvious solution, though Test written before, shown to produce known results, then refactor and shown to produce desired results (typically the same, depending on the refactor).
Yes - this was fixing some of the validation errors (#506), and would then have a separate PR for doing the abstraction.
Thanks! - I left one or two comments, and I'd want to review it again after it was merged into my branch. I'd consider the addition of this schema metadata to be a decorator - that is, the liveblog would work fine without it, but this just enhances it. As such, the logic to collate the data should be in its own class IMO, rather than making the liveblog.php even bigger. As I was looking into this, I then saw that (far too) many of the methods in the various classes seem to be I think that merging your branch into mine, and then re-checking mine and merging it here would make sense for now, but it doesn't address what I think is some technical debt that is causing a limitation on using good coding practices. |
💯 we have a bunch of technical debt to address, and we will :) |
Abstract the schema metadata for use outside AMP
This is the first step of fixing some of the issues in #506, and refactoring the code to aid with #505.
See individual commits for more info.