Podcast player: Render podcast header markup by server-side

[retrofox]

This PR renders the podcast header on the server-side.

Changes proposed in this Pull Request:

It gets the podcast data from the Podcast RSS and renders the block HTML on the server-side, following the same structure which is applied by the client.
It adds a simple template system in order to isolate the process, suggested by @getdave here.

Testing instructions:

In the front-end disable javascript. For this:

a) Open Chrome DevTools.
b) Press Control+Shift+P or Command+Shift+P (Mac) to open the Command Menu.
c) Start typing javascript, select Disable JavaScript, and then press Enter to run the command. JavaScript is now disabled.

Once it's disabled, do a hard-refresh. You should be able to still see the podcast header. Same HTML structure. Styles should be applied properly.

Proposed changelog entry for your changes:

• Render Podcast header markup in the server-side

[jetpackbot]

	Warnings
⚠️	pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 0c4b7a0

[obenland]

I'm not sure moving the functions into a new file is what @getdave had in mind in #15203 (review)
I'm equally unsure how we can templify them better, given the amount of conditionals we have here.

As it stands now, I'd consider it inferior to #15203.

[getdave]

Update: @retrofox to save you the time I pushed changes I was detailing below. This way you can throw them away without having wasted any time if you don't like them. Personally I feel that extracting all HTML rendering into dedicated "templates" rather than building HTML in functions (🤢 ) will make this all easier to maintain. But I appreciate I may be in a minority...

---

Wow thanks for the refactor @retrofox.

I'm not sure moving the functions into a new file is what @getdave had in mind in #15203 (review)

I think this is almost what I had in mind. We should aim to have a single "component" per template file. So you shouldn't really have any functions within the files within /templates. Thanks to the way render works you can now treat the individual files in /templates sort of like React components so there should be no need to include functions within the templates themselves.

I'm equally unsure how we can templify them better, given the amount of conditionals we have here.

I don't understand why the conditionals make any difference. Perhaps I'm missing something critical?

As it stands now, I'd consider it inferior to #15203.

I trust your judgement. However, would it be ok for you to explain why you consider it inferior? I assume because you don't like the output buffered approach (granted it's not the "WordPress way"!)?

[retrofox]

I'm not sure moving the functions into a new file is what @getdave had in mind in #15203 (review)

Personally I prefer this approach but also am not 100% sure about the simplicity that it could bring, though. However, the fact that to be able to isolate the templates is a winning point.

I'm equally unsure how we can templify them better, given the amount of conditionals we have here.

This is something needed with or without templates. @getdave pushed a nice commit which improves the usage of the templates.

[getdave]

Overall this is good, but I'm not sure about having to manual disable all these PHPCS rules all the time.

For now this is ok, but in the longer term I wonder whether we can be cleverer about this?

If you want to ship "as is" with the disable rules I'm cool with that, but we should consider having follow up Issues for the alternative approach I suggest in this review.

[getdave]

I wish there was a better way to do this?

It looks like we can set certain variables to be ignored from this UNdefinedVariable check.

https://github.com/sirbrillig/phpcs-variable-analysis#customization

I proposed we

• update the var from $data to $templateProps
• add a rule to the .phpcs.xml.dist under the validUndefinedVariableNames property to ignore the var $templateProps.
• update the code in the render so that it doesn't perform an extract() and simply passes the $templateProps array.
• the individual templates can then extract the data from $templateProps (or not) as required without having to worry about disabling phpcs warnings.

@jeherve what do you think about this? Having to litter each template with these disable rules feels really horrible.

[jeherve]

Yes, that sounds like a good plan!

[retrofox]

If you want to ship "as is" with the disable rules I'm cool with that

@getdave yes, I think it's shippable. Let's create a new card for this in our dashboard.

[getdave]

Let's ship what we have and followup as you say. Thanks for this 👍