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

Extract pattern metadata (title & description) #312

Merged
merged 10 commits into from
Apr 6, 2022
Merged

Extract pattern metadata (title & description) #312

merged 10 commits into from
Apr 6, 2022

Conversation

oandregal
Copy link
Contributor

@oandregal oandregal commented Apr 5, 2022

In WordPress/gutenberg#36751 Gutenberg added the ability to automatically register patterns that live in the /patterns directory. These patterns provide metadata via headers in the PHP file as in:

<?php
/**
 * Title: My heading
 * Description: My pattern description.
 * Slug: my-theme/my-heading
 * Categories: text
 */

From the existing metadata, we need the Title to be translatable. This PR adds support for looking up those patterns and extracting the Title string into the pot file.

How to test

  • Pull this PR and create a foo-theme directory at the top-level, which contains the following files:

style.css with at least these contents:

/*
Theme Name: foo theme
*/

patterns/my-pattern.php with at least these contents:

<?php

/**
 * Title: My pattern title.
 * Description: My pattern description.
 */
  • Execute
composer install
vendor/bin/wp i18n make-pot foo-theme
  • Verify that a new foo-theme/foo-theme.pot file has been generated and that contains the pattern title and the theme name. You should get something along these lines:
# some other file header metadata

#. Theme Name of the theme
msgid "foo theme"
msgstr ""

#: patterns/my-pattern.php
msgctxt "Pattern title"
msgid "My pattern title."
msgstr ""

#: patterns/my-pattern.php
msgctxt "Pattern description"
msgid "My pattern description."
msgstr ""

@oandregal oandregal requested a review from a team as a code owner April 5, 2022 08:58
@swissspidy
Copy link
Member

From the existing metadata, we need the Title to be translatable

For this to be actually useful, Gutenberg should also actually translate the title. I don't see any $pattern_data['title'] = translate_with_gettext_context( $pattern_data['title'], 'Title of the pattern' ); call in WordPress/gutenberg#36751

@oandregal
Copy link
Contributor Author

For this to be actually useful, Gutenberg should also actually translate the title. I don't see any $pattern_data['title'] = translate_with_gettext_context( $pattern_data['title'], 'Title of the pattern' );

Yeah, I need to follow-up with that as well :)

@oandregal
Copy link
Contributor Author

I've prepared the Gutenberg PR that uses this data at WordPress/gutenberg#40047 but I'm having some trouble to get the translated strings.

@swissspidy swissspidy changed the title Extract the Title from patterns that live in the /patterns Extract pattern metadata (title & description) Apr 5, 2022
@swissspidy
Copy link
Member

Pending some tests this looks good to go! 👍

@oandregal
Copy link
Contributor Author

Pending some tests this looks good to go! +1

Tests would need to wait until tomorrow morning :)

@oandregal
Copy link
Contributor Author

@swissspidy pushed the test.

src/MakePotCommand.php Outdated Show resolved Hide resolved
Copy link
Member

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Just one nit

Co-authored-by: Pascal Birchler <pascal.birchler@gmail.com>
@swissspidy swissspidy merged commit bcb1a81 into wp-cli:main Apr 6, 2022
@oandregal oandregal deleted the add/translate-php-patterns-metadata branch April 6, 2022 15:50
@schlessera schlessera added this to the 2.2.14 milestone Apr 6, 2022
$options = [
// Extract 'Title' and 'Description' headers from pattern files.
'wpExtractPatterns' => isset( $this->main_file_data['Theme Name'] ),
'include' => array_merge( $this->include, array( 'patterns' ) ),
Copy link
Contributor

@ocean90 ocean90 May 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@swissspidy Do you know why this hardcodes patterns here? Since these are PHP files they should be included by default. Now you explicitly have to exclude the patterns when you for example are using --include=wp-admin/*. to only include wp-admin strings.

Related: https://meta.trac.wordpress.org/changeset/11809

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the idea was to make this more efficient and only scan these files when needed.

But you're right that this is already covered by the call above. I'll create a PHP to combine these.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bazza pushed a commit to WordPress/wordpress.org that referenced this pull request May 2, 2022
… command.

Ensures that strings from Twenty Twenty-Two are not included in POT files for wp-admin.

See wp-cli/i18n-command#312 (review).

git-svn-id: https://meta.svn.wordpress.org/sites/trunk@11809 74240141-8908-4e6f-9713-ba540dce6ec7
bazza pushed a commit to WordPress/wordpress.org that referenced this pull request May 3, 2022
…irmly by ignoring the `wp-content` directory entirely for these admin contexts.

It appears that the command does not fully respect the `exclude` parameter if it believes there are strings that should be included. By excluding a higher-level directory, it never see's the potential included `patterns` folder.

Ensures that strings from Twenty Twenty-Two are not included in POT files for wp-admin.

Previously: [11809].
See wp-cli/i18n-command#312 (review)


git-svn-id: https://meta.svn.wordpress.org/sites/trunk@11810 74240141-8908-4e6f-9713-ba540dce6ec7
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

Successfully merging this pull request may close these issues.

4 participants