-
Notifications
You must be signed in to change notification settings - Fork 798
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
Config Package: Allow for enabling post_list package #20938
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Beta plugin:
Jetpack plugin:
Debug Helper plugin:
Backup plugin:
Vaultpress plugin:
Boost plugin:
Search plugin:
|
a5b78bb
to
a6e9109
Compare
32351e5
to
debcc6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to add the package to the Jetpack plugin, adding a new item into this packages list, something like the following:
foreach (
array(
'sync',
'jitm',
'post_list',
)
as $feature
) {
$config->ensure( $feature );
}
However, for some reason, getting this notice:
[16-Sep-2021 10:09:04 UTC] PHP Notice: Unable to load class Automattic\Jetpack\Post_List.
Please add the package that contains it using composer and make sure you are requiring the Jetpack autoloader
in /usr/local/src/jetpack-monorepo/projects/packages/config/src/class-config.php on line 129
composer.json
and composer.lock
are updated into the Jetpack plugin folder, too.
@retrofox Thanks for catching that! I needed to load the class as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I've tested adding the package to the Jetpack plugins adding the item into the configure method:
public function configure() {
$config = new Config();
foreach (
array(
'sync',
'jitm',
'post_list', // <- new post list added to Jetpack plugin.
)
as $feature
) {
$config->ensure( $feature );
}
The package is properly loaded and it shows the image thumbnail.
Just a couple of notes:
Go to http://localhost/wp-admin/edit.php?post-list=true and check that the post thumbnails appear.
It seems we removed the post-list
flag from the query string, meaning the feature will be available simply on the http://localhost/wp-admin/edit.php
page. Is it correct?
Have we removed the no-thumb image when no thumbnail for the posts?
That's correct! I removed the flag from the testing instructions.
Looks like it's not showing up anymore. To be addressed here: #20889 |
Although we are able to merge it, I'd like to know the opinion of Jetpack folks |
7d8d948
to
9be09e3
Compare
Yes, the idea is that the decision of whether to load the package or not should be made by the plugin or wherever it's being used. If we didn't want to load it then we would not include the |
Fixes #21102
Part of #20941
Changes proposed in this Pull Request:
Jetpack product discussion
N/A
Does this pull request change what data or activity we track or use?
N/A
Testing instructions:
cd projects/plugins/jetpack && composer require automattic/jetpack-post-list
composer require automattic/jetpack-config
pnpm install
jetpack install -r && jetpack install --all
Create a file named 0-functions.php in the tools/docker/mu-plugins/ directory and add:
jetpack build packages/post-list
jetpack docker up