Skip to content

Conversation

@prab18hat
Copy link
Contributor

  • Remove dead code related to WPT_CERTIFICATE_VALIDATION in prepare.php
  • Remove WPT_CERTIFICATE_VALIDATION processing from setup_runner_env_vars()
  • Update PHPDoc to reflect removed functionality
  • Fixes issue WPT_CERTIFICATE_VALIDATION is processed but never used #265 where environment variable was processed but never used

- Remove dead code related to WPT_CERTIFICATE_VALIDATION in prepare.php
- Remove WPT_CERTIFICATE_VALIDATION processing from setup_runner_env_vars()
- Update PHPDoc to reflect removed functionality
- Fixes issue WordPress#265 where environment variable was processed but never used
@github-actions
Copy link

github-actions bot commented Sep 27, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: prab18hat <prab18hat@git.wordpress.org>
Co-authored-by: desrosj <desrosj@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@prab18hat
Copy link
Contributor Author

kindly check my latest changes?

Copy link
Member

@desrosj desrosj left a comment

Choose a reason for hiding this comment

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

Hi @prab18hat! Thanks for this. I haven't gotten to test, but the PR looks good at first glance.

Before merging, I would like to get a better understanding around the history of the setting.

  • Was it ever used correctly?
  • Does the scenario it was meant to address still exist?
  • etc.

@prab18hat
Copy link
Contributor Author

Hi @prab18hat! Thanks for this. I haven't gotten to test, but the PR looks good at first glance.

Before merging, I would like to get a better understanding around the history of the setting.

  • Was it ever used correctly?
  • Does the scenario it was meant to address still exist?
  • etc.

Hey @desrosj!

Thanks for the thorough review! Your questions about the history are spot on - it's always good to understand the "why" behind changes like this.

I spent some time digging through the code and commit history to give you a complete answer:

Looking at the git history, the WPT_CERTIFICATE_VALIDATION variable was introduced to handle TLS certificate validation when downloading files (likely via wget with the --no-check-certificate flag). However, from what I can see in the code, it was never actually implemented correctly - the variable was being processed and stored in $runner_vars but then never used anywhere in the actual operations.
>The original use case was probably for local development environments where self-signed certificates might cause issues. But looking at the current codebase, we're not doing any wget operations that would need certificate validation flags - we're mainly using git clone (which uses HTTPS but handles certs differently) and rsync for file operations.

Issue #265 correctly identified that this was processed but never used. Rather than trying to figure out where it should have been used (and potentially introducing bugs), it made more sense to clean up the unused code. If someone actually needs certificate validation control in the future, they can add it back with proper implementation.

@desrosj
Copy link
Member

desrosj commented Sep 27, 2025

Understanding when it was introduced and removed is important to understand because it's possible that there are bugs even if none have been reported.

It looks like this was introduced in 559984c and removed in c3bdc43.

Disabling certificate validation was only ever used for downloading the WordPress Importer plugin. Since that's no longer necessary, this can safely be removed.

@prab18hat
Copy link
Contributor Author

Understanding when it was introduced and removed is important to understand because it's possible that there are bugs even if none have been reported.

It looks like this was introduced in 559984c and removed in c3bdc43.

Disabling certificate validation was only ever used for downloading the WordPress Importer plugin. Since that's no longer necessary, this can safely be removed.

@desrosj Perfect! Thanks for digging into those specific commits - that's exactly the context I was looking for but couldn't find in my initial search. Great to have confirmation that it was specifically for the WordPress Importer plugin download, which explains why it became unused dead code.

Appreciate you taking the time to research this thoroughly! 👍

@kittenkamala kittenkamala merged commit dd0ea5c into WordPress:master Oct 1, 2025
4 checks passed
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.

3 participants