-
Notifications
You must be signed in to change notification settings - Fork 831
Forms: version iAPI modules with asset file hash instead of Jetpack version #45193
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
base: trunk
Are you sure you want to change the base?
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 4 files.
|
* @param string $asset_file Path to view.asset.php file. | ||
* @return string either asset hash or Jetpack version. | ||
*/ | ||
public static function get_view_asset_version( $asset_file ) { |
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 could see this in Jetpack main plugin's Assets
package as well.
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.
Why view
in the method name? Won't it work for any assets.php
file?
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.
Yep! It does.
I'm thinking of renaming to better highlight the intent of using alongside wp_enqueue_script_module()
. Or just leave it generic.
*/ | ||
public static function get_view_asset_version( $asset_file ) { | ||
$asset = file_exists( $asset_file ) ? require $asset_file : null; | ||
$jetpack_version = Constants::get_constant( 'JETPACK__VERSION' ); |
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.
Alternatively we could fail loudly instead of falling back to JP version; now if the path is incorrect for example, dev might not notice.
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.
Failing when we actually expect the file to exist is better IMHO.
projects/packages/forms/src/contact-form/class-contact-form-field.php
Outdated
Show resolved
Hide resolved
5d96c9b
to
363abbe
Compare
Co-authored-by: Manzoor Wani <manzoorwani.jk@gmail.com>
Another consideration; in earlier PR we had used similar generated version for each asset, but we really need it only for modules. Should we use it everywhere or just focus on fields with modules? |
Resolves FORMS-287
Alternative to #45169
Proposed changes:
view.asset.php
file, falling back to Jetpack versionBefore
After
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
view.asset.php
files