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

Move WPCOM and Jetpack-specific codebase into the Jetpack plugin #1021

Closed
westonruter opened this issue Mar 15, 2018 · 11 comments
Closed

Move WPCOM and Jetpack-specific codebase into the Jetpack plugin #1021

westonruter opened this issue Mar 15, 2018 · 11 comments
Milestone

Comments

@westonruter
Copy link
Member

As we work to add AMP support to the WordPress ecosystem it is not feasible for the AMP plugin itself to support all of the plugins. Instead, the plugins themselves should be supporting AMP. So the current Jetpack-specific code should be moved into the AMP plugin. The new Lazy Images feature of Jetpack actually was built with AMP-support baked in.

Code in scope for moving to Jetpack:

  • Jetpack-specific embeds.
  • /jetpack-helper.php
  • /wpcom
  • /wpcom-helper.php
@postphotos
Copy link
Contributor

postphotos commented Mar 16, 2018

Hey @westonruter , I've taken a crack at writing a story and some ACs out of this. Let me know if I'm missing anything.

As a WordPress user, I should expect each plugin to contain AMP-specific code in its own codebase, not in the main AMP-WP plugin.

  • AC1: Add appropriate AMP support directly to the Jetpack plugin if necessary.
  • AC2: Remove Jetpack files from AMP-WP plugin.
  • AC3: Validate that all Jetpack AMP-compatible features that work before this adjustment still work.
  • AC4: Add basic documentation to the Wiki that outlines why we don't include direct plugin support and gives specific direction to a plugin developer looking to add AMP support (if needed) to their plugin.

I've taken the liberty of adding AC4, let me know if you think it's necessary.

@philipjohn
Copy link
Contributor

/wpcom-helper.php

I wouldn't class this under the same umbrella as Jetpack, because it's specific to the WordPress.com VIP environment. We also have vipgo-helper.php files for VIP Go.

We (VIP) do need a better way of inserting our environment-specific modification than including in plugins although the solution we end up with will likely only get applied to VIP Go. Regardless, it's not something that really belongs in Jetpack either.

For now, with plugins like Liveblog, we version control the helpers in Github and then exclude from the WordPress.org releases.

@amedina
Copy link
Member

amedina commented Apr 24, 2018

It is important to do this to decouple the plugin from any dependencies. If the code is not moved to Jetpack, what would the plan be?

@westonruter
Copy link
Member Author

It sounds like the code should be incorporated into WordPress.com's platform codebase and not made part of any plugin then.

@gravityrail
Copy link
Contributor

I moved some wpcom-helper features to Jetpack in Automattic/jetpack#9458. AMP plugin side is #1149

@gravityrail
Copy link
Contributor

The Jetpack side is merged, so I'm just waiting on someone to review #1149 against the Jetpack master branch (6.2)

@westonruter
Copy link
Member Author

westonruter commented Mar 5, 2019

@jeherve Is it safe now to remove jetpack-helper.php and wpcom-helper.php? See see #1925.

@jeherve
Copy link
Contributor

jeherve commented Mar 5, 2019

Yup, it looks safe now!

@westonruter
Copy link
Member Author

@jeherve Just to be clear: all of the code in wpcom-helper.php has been copied to WordPress.com (including Go)?

@jeherve
Copy link
Contributor

jeherve commented Mar 5, 2019

My bad, I only looked at jetpack-helper.php in #1925, I did not check wpcom-helper.php.

I am not familiar with the file, so it may be best to ask someone who is more familiar with it. cc @mjangda Do you think you could take a look at this?

@gravityrail
Copy link
Contributor

@jeherve and @westonruter - the wpcom helper is no longer used on wpcom as far as I can tell. It's not required anywhere.

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

No branches or pull requests

6 participants