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.

Sign up for GitHub

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

Synced Newspack Blocks: Update to 4.5.2 #40636

Merged
merged 27 commits into from
Dec 19, 2024
Merged

Synced Newspack Blocks: Update to 4.5.2 #40636

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
f8fb1e9
Sync newspack-blocks to 4.5.2
tbradsha Dec 16, 2024
73e653f
Update script to auto-add textdomain to block.json
tbradsha Dec 16, 2024
7520915
Update script to detect potential ENT_COMPAT needs
tbradsha Dec 16, 2024
26ed945
Script tweaks
tbradsha Dec 16, 2024
f51d4b3
More script tweaks
tbradsha Dec 16, 2024
e385469
Add .phpcs.dir.xml via script
tbradsha Dec 16, 2024
58c59a2
Finish up 4.5.2 block sync
tbradsha Dec 16, 2024
428a09d
Update baseline
tbradsha Dec 16, 2024
c1caf91
Update Phan baseline in script
tbradsha Dec 16, 2024
de03fc9
Commit changes from script
tbradsha Dec 16, 2024
3af28b0
Add ENT_COMPAT again
tbradsha Dec 16, 2024
bdee6ad
Add changelog
tbradsha Dec 16, 2024
307bbd4
Add .phpcsignore
tbradsha Dec 17, 2024
f334dbe
Commit synced blocks with PHPCS lint disabled
tbradsha Dec 17, 2024
0b0dba3
Allow one to resync current version
tbradsha Dec 17, 2024
ba51ed5
Update eslint and phpcbf calls
tbradsha Dec 17, 2024
83b0491
Commit less-linted JS/TS files
tbradsha Dec 17, 2024
3135249
Clean up paths
tbradsha Dec 17, 2024
ed996dd
Once again apply ENT_COMPAT changes
tbradsha Dec 17, 2024
de0c564
Update README
tbradsha Dec 17, 2024
c354232
Update index.php version number via script
tbradsha Dec 17, 2024
877a79d
Don't run Phan on synced files
tbradsha Dec 17, 2024
38dafc1
Remove Phan from README
tbradsha Dec 17, 2024
8465bfc
Simplify PHP textdomain fix
tbradsha Dec 18, 2024
5d3d690
Catch additional textdomain cases
tbradsha Dec 19, 2024
2f56f6b
Adjust docs wording
tbradsha Dec 19, 2024
bc7ac12
Remove `ENT_COMPAT` step
tbradsha Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions projects/packages/jetpack-mu-wpcom/.phan/baseline.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
// PhanTypeArraySuspiciousNullable : 6 occurrences
// PhanNoopNew : 5 occurrences
// PhanParamTooMany : 4 occurrences
// PhanTypeMismatchArgumentInternal : 4 occurrences
// PhanUnextractableAnnotationSuffix : 4 occurrences
// PhanDeprecatedProperty : 3 occurrences
// PhanPluginDuplicateExpressionAssignmentOperation : 3 occurrences
Expand All @@ -28,11 +29,10 @@
// PhanUndeclaredClassReference : 3 occurrences
// PhanUndeclaredGlobalVariable : 3 occurrences
// PhanEmptyFQSENInCallable : 2 occurrences
// PhanPluginMixedKeyNoKey : 2 occurrences
// PhanTypeArraySuspicious : 2 occurrences
// PhanTypeMismatchArgumentInternal : 2 occurrences
// PhanTypeMismatchDefault : 2 occurrences
// PhanTypeMissingReturn : 2 occurrences
// PhanCoalescingNeverNull : 1 occurrence
// PhanDeprecatedFunction : 1 occurrence
// PhanImpossibleTypeComparison : 1 occurrence
// PhanNonClassMethodCall : 1 occurrence
Expand Down Expand Up @@ -69,13 +69,13 @@
'src/features/marketplace-products-updater/class-marketplace-products-updater.php' => ['PhanTypeMismatchDimFetch', 'PhanTypeMismatchReturn'],
'src/features/media/heif-support.php' => ['PhanPluginSimplifyExpressionBool'],
'src/features/newspack-blocks/synced-newspack-blocks/blocks/carousel/view.php' => ['PhanParamTooMany', 'PhanPluginDuplicateConditionalNullCoalescing', 'PhanPossiblyUndeclaredVariable', 'PhanTypeMismatchArgument', 'PhanTypeMismatchReturnProbablyReal'],
'src/features/newspack-blocks/synced-newspack-blocks/blocks/homepage-articles/class-wp-rest-newspack-articles-controller.php' => ['PhanTypeArraySuspiciousNullable'],
'src/features/newspack-blocks/synced-newspack-blocks/blocks/homepage-articles/class-wp-rest-newspack-articles-controller.php' => ['PhanCoalescingNeverNull', 'PhanTypeArraySuspiciousNullable'],
tbradsha marked this conversation as resolved.
Show resolved Hide resolved
'src/features/newspack-blocks/synced-newspack-blocks/blocks/homepage-articles/templates/article.php' => ['PhanTypeMismatchArgument', 'PhanUndeclaredGlobalVariable'],
'src/features/newspack-blocks/synced-newspack-blocks/blocks/homepage-articles/templates/articles-list.php' => ['PhanUndeclaredGlobalVariable'],
'src/features/newspack-blocks/synced-newspack-blocks/blocks/homepage-articles/templates/articles-loop.php' => ['PhanUndeclaredGlobalVariable'],
'src/features/newspack-blocks/synced-newspack-blocks/blocks/homepage-articles/view.php' => ['PhanPluginDuplicateExpressionAssignmentOperation', 'PhanTypeArraySuspiciousNullable', 'PhanTypeMismatchArgumentNullableInternal', 'PhanTypeMismatchReturnProbablyReal', 'PhanUndeclaredClassMethod'],
'src/features/newspack-blocks/synced-newspack-blocks/class-newspack-blocks-api.php' => ['PhanParamTooMany', 'PhanTypeMismatchArgument', 'PhanTypeMismatchReturn', 'PhanTypeMismatchReturnProbablyReal', 'PhanUndeclaredClassMethod', 'PhanUndeclaredFunction', 'PhanUnextractableAnnotationSuffix'],
'src/features/newspack-blocks/synced-newspack-blocks/class-newspack-blocks.php' => ['PhanDeprecatedProperty', 'PhanPluginDuplicateConditionalNullCoalescing', 'PhanPluginDuplicateExpressionAssignmentOperation', 'PhanPluginMixedKeyNoKey', 'PhanPluginRedundantAssignment', 'PhanTypeInvalidLeftOperandOfBitwiseOp', 'PhanTypeInvalidRightOperandOfBitwiseOp', 'PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentNullable', 'PhanTypeMismatchProperty', 'PhanTypeMismatchReturn', 'PhanTypeMismatchReturnProbablyReal', 'PhanTypeSuspiciousNonTraversableForeach', 'PhanUndeclaredClassMethod', 'PhanUndeclaredClassReference', 'PhanUndeclaredConstant', 'PhanUndeclaredFunction'],
'src/features/newspack-blocks/synced-newspack-blocks/class-newspack-blocks.php' => ['PhanDeprecatedProperty', 'PhanPluginDuplicateConditionalNullCoalescing', 'PhanPluginDuplicateExpressionAssignmentOperation', 'PhanPluginRedundantAssignment', 'PhanTypeInvalidLeftOperandOfBitwiseOp', 'PhanTypeInvalidRightOperandOfBitwiseOp', 'PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentNullable', 'PhanTypeMismatchProperty', 'PhanTypeMismatchReturn', 'PhanTypeMismatchReturnProbablyReal', 'PhanTypeSuspiciousNonTraversableForeach', 'PhanUndeclaredClassMethod', 'PhanUndeclaredClassReference', 'PhanUndeclaredConstant', 'PhanUndeclaredFunction'],
'src/features/verbum-comments/class-verbum-comments.php' => ['PhanImpossibleTypeComparison', 'PhanNoopNew', 'PhanParamTooMany', 'PhanTypeMismatchArgumentProbablyReal', 'PhanUndeclaredFunction'],
'src/features/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-launchpad.php' => ['PhanPluginDuplicateConditionalNullCoalescing'],
'src/features/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-site-migration-migrate-guru-key.php' => ['PhanUndeclaredClassMethod'],
Expand Down
1 change: 1 addition & 0 deletions projects/packages/jetpack-mu-wpcom/.phpcsignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
src/features/newspack-blocks/synced-newspack-blocks
97 changes: 74 additions & 23 deletions projects/packages/jetpack-mu-wpcom/bin/sync-newspack-blocks.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ fi

# pick up value considering that the argument
# has the --key=value shape.
key_value=$(echo ${1} | cut -d'=' -f 2)
key_value=$(echo "${1}" | cut -d'=' -f 2)
# Set mode depending on first argument
if [[ $1 =~ ^--release= ]]
then
Expand Down Expand Up @@ -44,15 +44,14 @@ then
echo --branch=master
echo "--nodemodules (to use defined in package.json)"
echo "--path=/path/to/newspack-blocks"
echo --release=v2.0.0
echo --release=v4.0.0
echo
echo You can find the latest release ID on https://github.com/Automattic/newspack-blocks/releases/latest
echo
exit 1
fi

TARGET=./src/features/newspack-blocks/synced-newspack-blocks
ENTRY=./src/features/newspack-blocks/index.php

if [[ ( "$MODE" != "path" ) && ( "$MODE" != "npm" ) ]];
then
Expand All @@ -62,20 +61,23 @@ then

if [[ "$CURRENT_VERSION" == "$NAME" ]]; then
echo "The current version $CURRENT_VERSION of the newspack-blocks is synced."
exit 0
read -rp "Do you want to proceed anyway? (y/N): " proceed
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want you could source the file from the monorepo root tools/includes/proceed_p.sh to use our fancy function for doing prompts like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any advantage of sourcing vs a simple read? :^)

Copy link
Contributor

Choose a reason for hiding this comment

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

The proceed_p function handles a bunch of edge cases. It doesn't matter a whole lot though. 🤷

if [[ ! "$proceed" =~ ^[Yy]$ ]]; then
exit 0
fi
fi
fi

# make a temp directory
TEMP_DIR=`mktemp -d`
TEMP_DIR=$(mktemp -d)
CODE=$TEMP_DIR/code

# download zip file
echo Downloading $MODE $NAME into $TEMP_DIR
(cd $TEMP_DIR && curl -L --fail -s -O $URL)
echo "Downloading $MODE $NAME into $TEMP_DIR"
(cd "$TEMP_DIR" && curl -L --fail -s -O "$URL")

# handle download error
ZIPS=( $TEMP_DIR/*.zip )
ZIPS=( "$TEMP_DIR"/*.zip )
ZIP=${ZIPS[0]}
if [ ! -f "$ZIP" ]; then
echo "Tried to download $URL"
Expand All @@ -90,13 +92,13 @@ then
fi

# extract zip
echo Extracting into $CODE
mkdir -p $CODE
unzip -q $ZIP -d $CODE
echo "Extracting into $CODE"
mkdir -p "$CODE"
unzip -q "$ZIP" -d "$CODE"

# find the main file and use its directory as the root of our source dir
MAIN_FILE=`find $CODE -name "newspack-blocks.php"`
CODE=`dirname $MAIN_FILE`
MAIN_FILE=$(find "$CODE" -name "newspack-blocks.php")
CODE=$(dirname "$MAIN_FILE")

# handle unzip error
if [ ! -f "$CODE/newspack-blocks.php" ]; then
Expand Down Expand Up @@ -128,30 +130,79 @@ mkdir -p $TARGET/shared
mkdir -p $TARGET/types

# copy files and directories
NEW_VERSION=v`jq -r .version $CODE/package.json`
NEW_VERSION=v$(jq -r .version "$CODE"/package.json)
echo "$NEW_VERSION" > $TARGET/version.txt
cp $CODE/includes/class-newspack-blocks-api.php $TARGET/
cp $CODE/includes/class-newspack-blocks.php $TARGET/
cp -R $CODE/src/blocks/homepage-articles $TARGET/blocks/
cp -R $CODE/src/blocks/carousel $TARGET/blocks/
cp -R $CODE/src/shared $TARGET/
cp -R $CODE/src/components $TARGET/
cp "$CODE"/includes/class-newspack-blocks-api.php $TARGET/
cp "$CODE"/includes/class-newspack-blocks.php $TARGET/
cp -R "$CODE"/src/blocks/homepage-articles $TARGET/blocks/
cp -R "$CODE"/src/blocks/carousel $TARGET/blocks/
cp -R "$CODE"/src/shared $TARGET/
cp -R "$CODE"/src/components $TARGET/

# Get Typescript working by copying the main type defs over.
cp $CODE/src/types/index.d.ts $TARGET/types/
cp "$CODE"/src/types/index.d.ts $TARGET/types/
# Function types need to be capitalized in our system. We only match " function"
# beginning with a space to avoid matching it as a substring. (Not perfect, but
# imperfections will be caught by CI with failing tsc, etc.)
sed "${sedi[@]}" -e "s| function| Function|g" "$TARGET/types/index.d.ts"

# Note: I would have used eslint-nibble, but it doesn't support autofixing via the CLI.
echo "Changing JS textdomain to match jetpack-mu-wpcom..."
pnpm [email protected] dlx eslint --no-ignore --rule '"@wordpress/i18n-text-domain":["error",{"allowedTextDomain":"jetpack-mu-wpcom"}]' --fix $TARGET > /dev/null
BASE=$(cd "$(dirname "${BASH_SOURCE[0]}")"/../../../.. && pwd)
FULLTARGET="$PWD/$TARGET"

# Add a temporary single-rule eslint.config.mjs file.
cat > "$TARGET/eslint.config.mjs" <<EOF
import makeBaseConfig from 'jetpack-js-tools/eslintrc/base.mjs';

// This directory is copy-pasted from elsewhere, but we still need to run this one rule over it so we can't just .eslintignore.
tbradsha marked this conversation as resolved.
Show resolved Hide resolved
export default [
// Import base config, but no rules.
...makeBaseConfig( import.meta.url ).map( block => ( { ...block, rules: {} } ) ),
// Enable just this one rule.
{
rules: {
"@wordpress/i18n-text-domain": [ "error", { allowedTextDomain: "jetpack-mu-wpcom" } ],
}
},
];
EOF
( cd "$BASE" && pnpm run lint-file --no-inline-config --no-ignore --fix "$FULLTARGET" )
rm "$TARGET/eslint.config.mjs"

echo "Changing JS translation function call to avoid bad minification..."
pnpm --package=jscodeshift dlx jscodeshift -t ./bin/sync-newspack-blocks-formatter.js --extensions=js $TARGET

# Add temporary PHPCS config file.
cat > "$TARGET/.phpcs.dir.xml" <<EOF
<?xml version="1.0"?>
<ruleset>
<rule ref="MediaWiki.Usage.ForbiddenFunctions">
<exclude name="MediaWiki.Usage.ForbiddenFunctions.isset"/>
</rule>
</ruleset>
EOF
echo "Changing PHP textdomain to match jetpack-mu-wpcom..."
../../../vendor/bin/phpcbf --standard=./.phpcs.dir.xml --filter=../../../vendor/automattic/jetpack-phpcs-filter/src/PhpcsFilter.php --runtime-set jetpack-filter-no-ignore -q $TARGET
"$BASE"/vendor/bin/phpcbf --standard=./.phpcs.dir.xml --filter="$BASE"/vendor/automattic/jetpack-phpcs-filter/src/PhpcsFilter.php --runtime-set jetpack-filter-no-ignore -q "$TARGET"
rm "$TARGET/.phpcs.dir.xml"
tbradsha marked this conversation as resolved.
Show resolved Hide resolved

# Add textdomain to block.json
echo "Adding textdomain to all block.json files..."
for block_json_file in "$TARGET"/blocks/*/block.json; do
TMPFILE=$(mktemp)
jq --tab '. += {"textdomain": "jetpack-mu-wpcom"}' "$block_json_file" > "$TMPFILE"
mv "$TMPFILE" "$block_json_file"
done

echo "Updating Phan baseline..."
jetpack phan --update-baseline packages/jetpack-mu-wpcom
tbradsha marked this conversation as resolved.
Show resolved Hide resolved

# Warn about the need to use ENT_COMPAT.
echo "Please ensure htmlentities and html_entity_decode use 'ENT_COMPAT'!"
ent_compat_needed=$(grep -rino 'html_entity_decode\|htmlentities' --include="$TARGET/*.php")
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to work.

Suggested change
ent_compat_needed=$(grep -rino 'html_entity_decode\|htmlentities' --include="$TARGET/*.php")
ent_compat_needed=$(grep -rino 'html_entity_decode\|htmlentities' --include="*.php" "$TARGET")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me:

--snip--

Please ensure htmlentities and html_entity_decode use 'ENT_COMPAT'!
Detected the below instances:
./src/features/newspack-blocks/synced-newspack-blocks/class-newspack-blocks.php:1104:html_entity_decode
./src/features/newspack-blocks/synced-newspack-blocks/class-newspack-blocks.php:1104:htmlentities

Sync done.

That said, it'll only detect files directly in said folder.

Either way, it should soon be a non-issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upon further thought, no need to wait to remove this: bc7ac12

if [[ -n $ent_compat_needed ]]; then
echo 'Detected the below instances:'
echo "$ent_compat_needed"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this be a bunch of noise, since it doesn't actually check if ENT_COMPAT is already in use?

Ideally we'd let PHPCompatibility.ParameterValues.NewHTMLEntitiesFlagsDefault detect it, but first they'd have to release a version of PHPCompatibility that contains that rule. 🙁

OTOH, will the .phpcsignore take care of ignoring that too during the CI run? Is that the only reason this is here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a bunch of noise

As far as I can see, it's only ever been one line to change, so I don't think it's too much noise. It's in there because the README said it was important to check for, and I figured it'd be helpful to point out places to check vs. assume the user knows what to look for. 🤷

will the .phpcsignore take care of ignoring that too during the CI run?

Yeah, the synced folder will be ignored by CI, so it's a manual fix we need to be aware of (though I'm open to alternatives).

That said, I wonder if this is an issue upstream too, at which point a patch there would remove the need to fix it here... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, if it needs fixing, the fix really should be upstreamed.

The consequences of not fixing it are that behavior will differ depending on the PHP version, PHP <8.1 vs 8.1+.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put in a PR upstream: Automattic/newspack-blocks#2010

Once that's merged, I'll remove that task from the script/README. :^)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed now instead of waiting: bc7ac12


echo Sync done.
4 changes: 4 additions & 0 deletions projects/packages/jetpack-mu-wpcom/changelog/update-synced_newspack_blocks
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: changed

Newspack Blocks: Updated to version 4.5.2.
8 changes: 0 additions & 8 deletions ...kages/jetpack-mu-wpcom/src/features/newspack-blocks/synced-newspack-blocks/.phpcs.dir.xml
View file
Open in desktop

This file was deleted.

40 changes: 20 additions & 20 deletions ...pcom/src/features/newspack-blocks/synced-newspack-blocks/blocks/carousel/create-swiper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ const autoplayClassName = 'wp-block-newspack-blocks-carousel__autoplay-playing';
/**
* A helper for IE11-compatible iteration over NodeList elements.
*
* @param {object} nodeList - List of nodes to be iterated over.
* @param {Function} cb - Invoked for each iteratee.
* @param {Object} nodeList List of nodes to be iterated over.
* @param {Function} cb Invoked for each iteratee.
*/
function forEachNode( nodeList, cb ) {
/**
Expand All @@ -29,7 +29,7 @@ function forEachNode( nodeList, cb ) {
/**
* Modifies attributes on slide HTML to make it accessible.
*
* @param {HTMLElement} slide - Slide DOM element
* @param {HTMLElement} slide Slide DOM element
*/
function activateSlide( slide ) {
if ( slide ) {
Expand All @@ -41,7 +41,7 @@ function activateSlide( slide ) {
/**
* Modifies attributes on slide HTML to make it accessible.
*
* @param {HTMLElement} slide - Slide DOM element
* @param {HTMLElement} slide Slide DOM element
*/
function deactivateSlide( slide ) {
if ( slide ) {
Expand All @@ -54,15 +54,15 @@ function deactivateSlide( slide ) {
* Creates a Swiper instance with predefined config used by the Articles
* Carousel block in both front-end and editor.
*
* @param {object} els - Swiper elements
* @param {Element} els.block - Block element
* @param {Element} els.container - Swiper container element
* @param {Element} els.next - Next button element
* @param {Element} els.prev - Previous button element
* @param {Element} els.play - Play button element
* @param {Element} els.pause - Pause button element
* @param {Element} els.pagination - Pagination element
* @param {Object} config - Swiper config
* @param {Object} els Swiper elements
* @param {Element} els.block Block element
* @param {Element} els.container Swiper container element
* @param {Element} els.next Next button element
* @param {Element} els.prev Previous button element
* @param {Element} els.play Play button element
* @param {Element} els.pause Pause button element
* @param {Element} els.pagination Pagination element
* @param {Object} config Swiper config
* @return {Object} Swiper instance
*/
export default function createSwiper( els, config = {} ) {
Expand Down Expand Up @@ -161,13 +161,13 @@ export default function createSwiper( els, config = {} ) {
escapeHTML(
`${ currentSlide.innerText },
${
alt
? /* translators: the title of the image. */ sprintf(
__( 'Image: %s, ', 'jetpack-mu-wpcom' ),
alt
)
: ''
}
alt
? /* translators: the title of the image. */ sprintf(
__( 'Image: %s, ', 'jetpack-mu-wpcom' ),
alt
)
: ''
}
${ slideInfo }`
),
'assertive'
Expand Down
Loading
Loading