Update Jetpack to use new JITM package #12516
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant. Rules triggering this positive scan:
cc: @pesieminski |
Collaborator
This is an automated check which relies on |
enejb
reviewed
May 30, 2019
enejb
reviewed
May 30, 2019
enejb
reviewed
May 30, 2019
enejb
reviewed
May 30, 2019
enejb
reviewed
May 30, 2019
enejb
reviewed
May 30, 2019
Contributor
|
This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant. Rules triggering this positive scan:
cc: @pesieminski |
dereksmart
force-pushed
the
feature/jetpack-packages
branch
from
7a4d3f3 to
7df90c1
Compare
Contributor
|
I wonder if it's worth merging this to the feature branch and then continuing to iterate in additional small PRs? It has dependencies on unpackaged files (e.g. the |
lezama
reviewed
May 31, 2019
| require_once( JETPACK__PLUGIN_DIR . 'class.jetpack-jitm.php' ); | ||
|
|
||
| $jitm = Jetpack_JITM::init(); | ||
| $jitm = new JITM_Manager(); |
Contributor
There was a problem hiding this comment.
I think packages should be responsible of registering their API endpoints.
I would move all this code to the package, what do you all think?
Contributor
Author
There was a problem hiding this comment.
Sounds good, I will do it in the next iteration.
lezama
reviewed
May 31, 2019
lezama
reviewed
May 31, 2019
Contributor
|
Caution: This PR has changes that must be merged to WordPress.com |
tyxla
reviewed
Jun 4, 2019
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Nice work with this one so far 👏
I've left some questions and thoughts, seems like it needs a solid rebase and some updates to it consumes the latest packages we have in the feature branch.
Also, I was wondering - are we planning to introduce JITM tests as part of this PR?
| * | ||
| * @return string | ||
| */ | ||
| public function get_jp_emblem( $logotype = false ) { |
Member
There was a problem hiding this comment.
Could we move this in a separate PR, as it touches a different package?
Contributor
Author
There was a problem hiding this comment.
Dan had asked that we try to get everything into feature/jetpack-packages as quickly as we can. Where the work is required to remove dependencies in JITM, has already been done in this PR, and is all going to the same place, I think it makes sense to merge it as part of this PR.
| * @return string | ||
| */ | ||
| public function get_jp_emblem( $logotype = false ) { | ||
| $logo = '<path fill="#00BE28" d="M16,0C7.2,0,0,7.2,0,16s7.2,16,16,16c8.8,0,16-7.2,16-16S24.8,0,16,0z M15.2,18.7h-8l8-15.5V18.7z M16.8,28.8 V13.3h8L16.8,28.8z"/>'; |
Member
There was a problem hiding this comment.
Should we read from an svg file instead?
Contributor
Author
There was a problem hiding this comment.
This is the way it was done previously. I'm not against refactoring things further, but I would prefer to do things one step at a time, i.e. get things ported to independent packages successfully and then refactor them further.
| '; | ||
|
|
||
| return sprintf( | ||
| '<svg id="jetpack-logo__icon" xmlns="http://www.w3.org/2000/svg" x="0px" y="0px" viewBox="0 0 %1$s 32">%2$s</svg>', |
Member
There was a problem hiding this comment.
What if we want to use multiple logos in the same page - should we remove the id?
Contributor
Author
There was a problem hiding this comment.
I copied this as is, so it's presumably working in its current state. I would prefer to get things refactored into independent packages successfully before changing functionality unrelated to that end.
| class Jetpack_JITM { | ||
| class Manager { | ||
|
|
||
| const PACKAGE_VERSION = '1.0'; |
Member
There was a problem hiding this comment.
That would add overhead. Since we use this only for caching assets, could we use filemtime() of the asset file for the version instead?
Contributor
Author
There was a problem hiding this comment.
I would prefer to keep the code as close to how it was / worked originally during the process of getting everything to work as independent packages, and then we can go back and make further changes as opposed to changing many things at once.
| ); | ||
|
|
||
| Jetpack_Options::update_option( 'hide_jitm', $hide_jitm ); | ||
| \Jetpack_Options::update_option( 'hide_jitm', $hide_jitm ); |
Member
There was a problem hiding this comment.
Should we use the options package here?
Contributor
Author
There was a problem hiding this comment.
That's the plan, just want to get what we have so far into features/jetpack-packages and continue to iterate
| ); | ||
|
|
||
| $normalized_site_url = Jetpack::build_raw_urls( get_home_url() ); | ||
| $normalized_site_url = \Jetpack::build_raw_urls( get_home_url() ); |
Member
There was a problem hiding this comment.
What are we planning to do with references to the Jetpack class, considering that packages are supposed to be independent?
Contributor
Author
There was a problem hiding this comment.
We'll continue to iterate on it and remove dependencies in another PR once it's been merged into the feature branch.
| /** | ||
| * Return string containing the Jetpack logo. | ||
| * | ||
| * @since 7.5.0 |
Member
There was a problem hiding this comment.
Having a @since with the Jetpack version here is confusing - is this the package version, or the Jetpack version? We could remove it, but it would also make sense to keep the package version, or even better, specify what version specifically we're talking about here (Jetpack or package version).
Contributor
Author
There was a problem hiding this comment.
Good point. I suppose we're going to need to start thinking in terms of package versions instead of Jetpack versions, since Jetpack is going to be a collection of packages. For that reason, I think it should be the package version.
Contributor
There was a problem hiding this comment.
@gititon let's write a quick PR or open issues for things like these, so these don't get lost in the chaos
Contributor
|
This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant. Rules triggering this positive scan:
cc: @pesieminski |
Contributor
|
This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant. Rules triggering this positive scan:
cc: @pesieminski |
matticbot
added
[Status] Needs Changelog
and removed
[Status] Needs Review
Member
|
Nice work with this one 👍 |
dereksmart
pushed a commit
that referenced
this pull request
Jun 10, 2019
* Add Custom Autoloader (#12447) * Try Custom Autoloader * spelling fix Co-Authored-By: Marin Atanasov <[email protected]> * minor space fix Co-Authored-By: Marin Atanasov <[email protected]> * minor space fix. Co-Authored-By: Marin Atanasov <[email protected]> * Updated to make it even simpler * test plugin * a Toy plugin :) 🚂 * Minor update * Minor update * Fixes typo * delete file * Minor updates * Added limitaionts section to readme * ooops * Fix readme * Turn the jetpack-autoloader into a package. * fixes to the jetpack a custom plugin * typo Co-Authored-By: Marin Atanasov <[email protected]> * remove scripts to "a" no longer needed thanks to @enejb magic * Update packages/autoloader/README.md fixes more wording in the readme Co-Authored-By: Miguel Lezama <[email protected]> * Update the version numbers of packages insead of branches * forgot about the plugin * update "a" composer * Update the a composer lock file. * Revert "Update the a composer lock file." This reverts commit 90f2cb8dbc50e15caf19fa0b88e1ee201f8f21c5. * typo fix Co-Authored-By: Miguel Lezama <[email protected]> * Rebased against master, testes that a plugin works as expected * Remove not needed code * Minor readme fix * Remoce Space * Add filter to prevent the autoloader from loading a particular version * add b toy plugin depending on Jetpack Logo 1.0.1 Co-Authored-By: Marin Atanasov <[email protected]> * minor space fix Co-Authored-By: Miguel Lezama <[email protected]> * Update a comment * Fix phpcs fixes * Make the remove class_map from the global name space, fix phpcs issues * Add the autoload_packages to be ignored. * Add debug info * Remove versions from composer.json of packages * Add missing vendor to namespace * Fix typo * Fix typo and add punctuation * Fix a typo * Update .gitignore Co-Authored-By: Jeremy Herve <[email protected]> * Update all the composer files * Take into account the @dev and always use the dev-* version * Bug Fix load the latest version! * Update the logic when enqueing the classes * Update to use Namespaces * Add tests to make sure that the enqueing works as expected * Fix suggestions from Marin * Remove generateClassMap method * Fix the tests, rename the fucntion name to be more accurate * More fixes to the tests. * FuN * Making it all work again * Update the lock file * Minor * Remove unused plugins (#12529) * Removed unnecessary gitignore lines. * Separate the connection library into its own package. (#12399) * Added an interface as a guide for the new package methods. * Add package structure * Initialized the composer library, added gitignore. * Removed phpunit from connection dependencies. * Added first real code to the connection manager class, as well as tests for it. * Renamed the test folder to tests. * Made the option name be a class constant. * Added 10up's WP_Mock as a dependency, refactored the injection to use filters instead of setters. * Changed the internal method accessibility to protected. * Initialized an empty package repo for the options manager. * Added the options package as a dependency to the master repo. * Added a package structure placeholder for the options manager. * Added the first test for the delete option method. * Added the update method and a test for it. * Added a test for the grouped option deletion. * Added the group updater method and a test for it. * Added the grouped option getter and a test for it. * Added a test for the noncompact option getter. * Added tests for network option manipulation. * Removed autoloader requirement in favour of the existing one. * Added the connection manager secret generator to actual Jetpack class. * Removed individual gitignore files in favor of a parent-level file, props @tyxla! * Moved packages source code into src folders. * Removed the V7 part of the namespace. * Remove v7 from namespace in the Manager Interface * Remove V7 from namespaces * Add Automattic to the namespace * Rename jetpack-[name] packages to [name] * Update composer lock * Adding a package for Jetpack Constants * Attempting to fix WPCOM tests. * Excluding packages folder from linting for PHP 5.6 and earlier. * adjust syntax in connection tests * removing new unit tests. i'm not sure why they are failing. we can re-add them later. * reverting last commit, and fixing tests in `packages/options` * ah, one more * remove an un-needed linting hack (#12543) * Composer: avoid duplicate repositories key (#12546) * Composer: avoid duplicate repositories key Rely on wildcard instead. See #12483 * Revert changes in composer.lock * Revert changes in composer.lock * Revert changes in composer.lock * Packages: Make logo package tests independent (#12514) * Move package tests to package directory * Add some dev deps to logo package * Ignore vendor of all packages * Add a logo package simple bootstrap * Add a logo package phpunit config * Cleanup package tests from Jetpack phpunit config * Remove WP prefix from class name * Make sure composer is installed * Try: add SCOPE to travis matrix * Try: Disable older PHP versions * Replace WP_Mock with PHPMock * Enable php 7.0 in Travis again * Make phpunit version more permissive * Fix setUp signature * Play with phpunit versions * Try enabling CI for feature branch * Try enabling CI for feature branch * Try enabling CI for feature branch * Revert latest changes * Try adding the branch to the whitelist * Enable PHP 5.6 again * Use php-mock instead of php-mock-phpunit * Run composer phpunit for all packages * Make mocking method protected * Add parent teardown * Remove whitelisted branch * Run only for packages with tests * remove trailing slash 🤔 * nope * not sure what I am doing * add debug statements * run only if we have `php` tests @zinigor @tyxla we need to normalise de connection one * Jetpack Options Package: just class map existing options class (#12562) * instead of a new implementation of jetpack options, let's class map it * Packages: Fix connection tests and allow them to run in CI (#12560) * Use php-mock instead of wp_mock * Add phpunit script * Migrate tests from wp_mock to phpmock * Make tests run in CI * Run travis only for packages that have the php test dir * Revert "Jetpack Options Package: just class map existing options class (#12562)" (#12571) This reverts commit 0e3ac7a. * Packages: Finish the constants package (#12558) * PSR-4 constants * Actually add namespacing to the constants class * Update composer.lock * Fix phpcs errors and add some docs * Add a package README * Move constant tests to the package * Update Jetpack to use the constants package * Fix constants usage in options package * Rename README variable Co-Authored-By: Miguel Lezama <[email protected]> * Fix typo Co-Authored-By: Miguel Lezama <[email protected]> * Use plural Co-Authored-By: Miguel Lezama <[email protected]> * Making Jetpack_Options a classmapped package. (#12577) * Making Jetpack_Options a classmapped package. * Update Jetpack to use new JITM package (#12516) * Update Jetpack to use new JITM package * Add instructions on package installation and usage (#12576) * Add instructions on package installation and usage * Move content about installing Composer to the doc regarding development environment. Add a pointer to that section in the packages' readme * Explain how to install Composer on systems other than macOS. Props @aldavigdis * Update docs/development-environment.md Co-Authored-By: Jeremy Herve <[email protected]> * Apply suggestions from code review Co-Authored-By: Jeremy Herve <[email protected]> * Add reference to Composer installation in the section that explains how to start developing for Jetpack * Packages: Fix alias example in README (#12582) * Packages: Move sync to a classmapped package (#12572) * Move all sync files to the package * Add sync package composer.json * Composer require the new sync package * Update composer.lock after adding sync package dependency * Remove some sync file requiring in sync package * Remove some sync file requiring in the rest of Jetpack * Hook Jetpack actions properly * Fix a rebase conflict * Fix update post terms test to not compare against the DB * Packages: Move JITM tests to package and fix deps (#12583) * Add phpunit devdep and composer phpunit script * Add JITM tests config * Move tests and add bootstrap * Remove legacy jitm suite * Rely on @dev connection package * Remove version * Update minimum stability and prefer stable * Fix tests * Update composer.lock * Fix whitespace Co-Authored-By: Miguel Lezama <[email protected]> * Prepend an empty line Co-Authored-By: Miguel Lezama <[email protected]> * Connection Package: add Jetpack_Data methods (#12580) Jetpack_Data -> Connection package * removes php_bug_66229_check method - it's no longer used in the code base. * Moving is_usable_domain to Connection package. * Moves `get_access_token` to Connection package. * Create package for Jetpack Tracking (#12596) Created package for Jetpack Tracking * Update/package logo add gray (#12610) * Update the logo package to include a gray version. Add parameter to render method to specify the type * Remove svg files. Update method to return an SVG string that can be stylized with CSS. Update tests. * Fixed issue with logo in Jetpack connection banner in WP Admin * Update test so it strictly tests two instances of the CSS class * Optimize performance. Use const since the string is immutable. Use nowdoc to compose the string. * Update logo tests in new file * Constants_Manager -> Constants * inc/lib/admin-pages/class.jetpack-react-page.php (#12627) * Add jetpack signature class to connection package (#12620) * Connection package: Add class.jetpack-signature and class.jetpack-error to connection package * DNA: Don't require composer autoloader (#12624) * Jitm package, don't use global Jetpack class (#12629)
kraftbj
pushed a commit
that referenced
this pull request
Dec 9, 2019
* Add Custom Autoloader (#12447) * Try Custom Autoloader * spelling fix Co-Authored-By: Marin Atanasov <[email protected]> * minor space fix Co-Authored-By: Marin Atanasov <[email protected]> * minor space fix. Co-Authored-By: Marin Atanasov <[email protected]> * Updated to make it even simpler * test plugin * a Toy plugin :) 🚂 * Minor update * Minor update * Fixes typo * delete file * Minor updates * Added limitaionts section to readme * ooops * Fix readme * Turn the jetpack-autoloader into a package. * fixes to the jetpack a custom plugin * typo Co-Authored-By: Marin Atanasov <[email protected]> * remove scripts to "a" no longer needed thanks to @enejb magic * Update packages/autoloader/README.md fixes more wording in the readme Co-Authored-By: Miguel Lezama <[email protected]> * Update the version numbers of packages insead of branches * forgot about the plugin * update "a" composer * Update the a composer lock file. * Revert "Update the a composer lock file." This reverts commit 90f2cb8dbc50e15caf19fa0b88e1ee201f8f21c5. * typo fix Co-Authored-By: Miguel Lezama <[email protected]> * Rebased against master, testes that a plugin works as expected * Remove not needed code * Minor readme fix * Remoce Space * Add filter to prevent the autoloader from loading a particular version * add b toy plugin depending on Jetpack Logo 1.0.1 Co-Authored-By: Marin Atanasov <[email protected]> * minor space fix Co-Authored-By: Miguel Lezama <[email protected]> * Update a comment * Fix phpcs fixes * Make the remove class_map from the global name space, fix phpcs issues * Add the autoload_packages to be ignored. * Add debug info * Remove versions from composer.json of packages * Add missing vendor to namespace * Fix typo * Fix typo and add punctuation * Fix a typo * Update .gitignore Co-Authored-By: Jeremy Herve <[email protected]> * Update all the composer files * Take into account the @dev and always use the dev-* version * Bug Fix load the latest version! * Update the logic when enqueing the classes * Update to use Namespaces * Add tests to make sure that the enqueing works as expected * Fix suggestions from Marin * Remove generateClassMap method * Fix the tests, rename the fucntion name to be more accurate * More fixes to the tests. * FuN * Making it all work again * Update the lock file * Minor * Remove unused plugins (#12529) * Removed unnecessary gitignore lines. * Separate the connection library into its own package. (#12399) * Added an interface as a guide for the new package methods. * Add package structure * Initialized the composer library, added gitignore. * Removed phpunit from connection dependencies. * Added first real code to the connection manager class, as well as tests for it. * Renamed the test folder to tests. * Made the option name be a class constant. * Added 10up's WP_Mock as a dependency, refactored the injection to use filters instead of setters. * Changed the internal method accessibility to protected. * Initialized an empty package repo for the options manager. * Added the options package as a dependency to the master repo. * Added a package structure placeholder for the options manager. * Added the first test for the delete option method. * Added the update method and a test for it. * Added a test for the grouped option deletion. * Added the group updater method and a test for it. * Added the grouped option getter and a test for it. * Added a test for the noncompact option getter. * Added tests for network option manipulation. * Removed autoloader requirement in favour of the existing one. * Added the connection manager secret generator to actual Jetpack class. * Removed individual gitignore files in favor of a parent-level file, props @tyxla! * Moved packages source code into src folders. * Removed the V7 part of the namespace. * Remove v7 from namespace in the Manager Interface * Remove V7 from namespaces * Add Automattic to the namespace * Rename jetpack-[name] packages to [name] * Update composer lock * Adding a package for Jetpack Constants * Attempting to fix WPCOM tests. * Excluding packages folder from linting for PHP 5.6 and earlier. * adjust syntax in connection tests * removing new unit tests. i'm not sure why they are failing. we can re-add them later. * reverting last commit, and fixing tests in `packages/options` * ah, one more * remove an un-needed linting hack (#12543) * Composer: avoid duplicate repositories key (#12546) * Composer: avoid duplicate repositories key Rely on wildcard instead. See #12483 * Revert changes in composer.lock * Revert changes in composer.lock * Revert changes in composer.lock * Packages: Make logo package tests independent (#12514) * Move package tests to package directory * Add some dev deps to logo package * Ignore vendor of all packages * Add a logo package simple bootstrap * Add a logo package phpunit config * Cleanup package tests from Jetpack phpunit config * Remove WP prefix from class name * Make sure composer is installed * Try: add SCOPE to travis matrix * Try: Disable older PHP versions * Replace WP_Mock with PHPMock * Enable php 7.0 in Travis again * Make phpunit version more permissive * Fix setUp signature * Play with phpunit versions * Try enabling CI for feature branch * Try enabling CI for feature branch * Try enabling CI for feature branch * Revert latest changes * Try adding the branch to the whitelist * Enable PHP 5.6 again * Use php-mock instead of php-mock-phpunit * Run composer phpunit for all packages * Make mocking method protected * Add parent teardown * Remove whitelisted branch * Run only for packages with tests * remove trailing slash 🤔 * nope * not sure what I am doing * add debug statements * run only if we have `php` tests @zinigor @tyxla we need to normalise de connection one * Jetpack Options Package: just class map existing options class (#12562) * instead of a new implementation of jetpack options, let's class map it * Packages: Fix connection tests and allow them to run in CI (#12560) * Use php-mock instead of wp_mock * Add phpunit script * Migrate tests from wp_mock to phpmock * Make tests run in CI * Run travis only for packages that have the php test dir * Revert "Jetpack Options Package: just class map existing options class (#12562)" (#12571) This reverts commit 0e3ac7a. * Packages: Finish the constants package (#12558) * PSR-4 constants * Actually add namespacing to the constants class * Update composer.lock * Fix phpcs errors and add some docs * Add a package README * Move constant tests to the package * Update Jetpack to use the constants package * Fix constants usage in options package * Rename README variable Co-Authored-By: Miguel Lezama <[email protected]> * Fix typo Co-Authored-By: Miguel Lezama <[email protected]> * Use plural Co-Authored-By: Miguel Lezama <[email protected]> * Making Jetpack_Options a classmapped package. (#12577) * Making Jetpack_Options a classmapped package. * Update Jetpack to use new JITM package (#12516) * Update Jetpack to use new JITM package * Add instructions on package installation and usage (#12576) * Add instructions on package installation and usage * Move content about installing Composer to the doc regarding development environment. Add a pointer to that section in the packages' readme * Explain how to install Composer on systems other than macOS. Props @aldavigdis * Update docs/development-environment.md Co-Authored-By: Jeremy Herve <[email protected]> * Apply suggestions from code review Co-Authored-By: Jeremy Herve <[email protected]> * Add reference to Composer installation in the section that explains how to start developing for Jetpack * Packages: Fix alias example in README (#12582) * Packages: Move sync to a classmapped package (#12572) * Move all sync files to the package * Add sync package composer.json * Composer require the new sync package * Update composer.lock after adding sync package dependency * Remove some sync file requiring in sync package * Remove some sync file requiring in the rest of Jetpack * Hook Jetpack actions properly * Fix a rebase conflict * Fix update post terms test to not compare against the DB * Packages: Move JITM tests to package and fix deps (#12583) * Add phpunit devdep and composer phpunit script * Add JITM tests config * Move tests and add bootstrap * Remove legacy jitm suite * Rely on @dev connection package * Remove version * Update minimum stability and prefer stable * Fix tests * Update composer.lock * Fix whitespace Co-Authored-By: Miguel Lezama <[email protected]> * Prepend an empty line Co-Authored-By: Miguel Lezama <[email protected]> * Connection Package: add Jetpack_Data methods (#12580) Jetpack_Data -> Connection package * removes php_bug_66229_check method - it's no longer used in the code base. * Moving is_usable_domain to Connection package. * Moves `get_access_token` to Connection package. * Create package for Jetpack Tracking (#12596) Created package for Jetpack Tracking * Update/package logo add gray (#12610) * Update the logo package to include a gray version. Add parameter to render method to specify the type * Remove svg files. Update method to return an SVG string that can be stylized with CSS. Update tests. * Fixed issue with logo in Jetpack connection banner in WP Admin * Update test so it strictly tests two instances of the CSS class * Optimize performance. Use const since the string is immutable. Use nowdoc to compose the string. * Update logo tests in new file * Constants_Manager -> Constants * inc/lib/admin-pages/class.jetpack-react-page.php (#12627) * Add jetpack signature class to connection package (#12620) * Connection package: Add class.jetpack-signature and class.jetpack-error to connection package * DNA: Don't require composer autoloader (#12624) * Jitm package, don't use global Jetpack class (#12629)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves JITM related code to it's own composer package.
The new package still contains some threads of Jetpack-specific dependencies.
We'll need to circle back and fix them in a subsequent PR. See: #12579