close
Skip to content

Add PHP version check for plugin.#149

Merged
dkotter merged 11 commits into
developfrom
fix/148-php-ver-check
Jan 11, 2024
Merged

Add PHP version check for plugin.#149
dkotter merged 11 commits into
developfrom
fix/148-php-ver-check

Conversation

@rahulsprajapati
Copy link
Copy Markdown
Contributor

Description of the Change

Add minimum version check for the plugin before loading it. This will ensure plugin update doesn't break the site that don't match our minimum PHP version.

image

Closes #148

How to test the Change

  1. Setup a WordPress environment running PHP 7.3
  2. Install ads.txt plugin by downloading it with this feature branch.
  3. Verify that plugin functionality doesn't load and you see an admin error message for php version. ( refer above screenshot in Description )

Changelog Entry

Fixed - Better error handling for environments that don't match our minimum PHP version

Credits

Props @dkotter, @rahulsprajapati

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@rahulsprajapati rahulsprajapati self-assigned this Jul 20, 2023
@jeffpaul
Copy link
Copy Markdown
Member

jeffpaul commented Oct 9, 2023

@peterwilsoncc looking for review here, thanks!

@jeffpaul jeffpaul added this to the 1.5.0 milestone Oct 9, 2023
Copy link
Copy Markdown
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

The version check works as expected but I've included a couple of notes inline, the main issue is avoiding the potential for syntax errors triggering fatals.

Comment thread ads-txt.php Outdated
Comment on lines +52 to +58
echo wp_kses_post(
sprintf(
/* translators: %s: Minimum required PHP version */
__( 'Ads.txt requires PHP version %s or later. Please upgrade PHP or disable the plugin.', 'ads-txt' ),
esc_html( adstxt_minimum_php_requirement() )
)
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As the translation doesn't include HTML, the kses call can be avoided by escaping the string.

Suggested change
echo wp_kses_post(
sprintf(
/* translators: %s: Minimum required PHP version */
__( 'Ads.txt requires PHP version %s or later. Please upgrade PHP or disable the plugin.', 'ads-txt' ),
esc_html( adstxt_minimum_php_requirement() )
)
);
printf(
/* translators: %s: Minimum required PHP version */
esc_html__( 'Ads.txt requires PHP version %s or later. Please upgrade PHP or disable the plugin.', 'ads-txt' ),
esc_html( adstxt_minimum_php_requirement() )
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Resolved here: 552f5a4

Comment thread ads-txt.php

require_once __DIR__ . '/inc/post-type.php';
require_once __DIR__ . '/inc/admin.php';
require_once __DIR__ . '/inc/save.php';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To avoid an issue similar to 10up/simple-page-ordering#163, the lines below this will need to be moved to a new file and moved to a file within the inc folder.

This will allow for the code to be updated to use modern PHP syntax without throwing a fatal error in older versions of PHP due to a syntax error.

Copy link
Copy Markdown
Contributor

@frankiebordone frankiebordone Jan 10, 2024

Choose a reason for hiding this comment

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

Code moved here:

Updates to function namespaces within new file:

Associated unit test update:

@frankiebordone
Copy link
Copy Markdown
Contributor

frankiebordone commented Jan 10, 2024

@peterwilsoncc Your previous feedback items on this PR have been addressed! Note: Some E2E related jobs were not successful as per https://github.com/10up/ads-txt/actions/runs/7477337138 - do we typically ignore these?

CC: @jeffpaul

Comment thread inc/helpers.php Outdated
*
* @return void
*/
function tenup_display_ads_txt() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Now that these functions have been moved to a file with a namespace, I'd suggest we remove any namespacing from the function names. This includes any of the tenup_ prefixes and could also remove some of the adstxt ones.

I'd suggest something like the following:

  • tenup_display_ads_txt -> display_ads_txt
  • add_adstxt_capabilities -> add_capabilities
  • remove_adstxt_capabilities -> remove_capabilities
  • tenup_ads_txt_add_query_vars -> add_query_vars

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @dkotter, I've made the suggested updates here: df602d2

@dkotter
Copy link
Copy Markdown
Collaborator

dkotter commented Jan 10, 2024

Note: Some E2E related jobs were not successful as per https://github.com/10up/ads-txt/actions/runs/7477337138 - do we typically ignore these

I have fixed these

@dkotter dkotter merged commit 275c3dd into develop Jan 11, 2024
@dkotter dkotter deleted the fix/148-php-ver-check branch January 11, 2024 20:14
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.

Add PHP checks

5 participants