Stop the phpversion fact raising without php by tobixen · Pull Request #768 · voxpupuli/puppet-php

tobixen · 2026-09-17T23:30:17Z

Pull Request (PR) description

Disclaimer: The code here is AI-generated, by Claude Opus 5. Having the AI generate pull requests is cheap, reviewing the code is not (though, this one should be really trivial). I respect that some maintainers may want to close AI-generated pull requests without spending time on explaining why. The real value I provide here is bug discovery, reproduction and testing.

This is a follow-up on commit f351625 from pull request #767, which reintroduces the problem reported in issue #134.

Claude was prompted to fix the problem with this puppet output:

(...)
Notice: /File[/opt/puppetlabs/puppet/cache/lib/facter/phpversion.rb]/content:
--- /opt/puppetlabs/puppet/cache/lib/facter/phpversion.rb    2026-03-20 12:15:28.775782952 +0000
+++ /tmp/puppet-file20260917-169843-1msbgfu    2026-09-17 21:31:11.107207208 +0000
@@ -2,7 +2,7 @@

 Facter.add(:phpversion) do
   setcode do
- output = Facter::Util::Resolution.exec('php -v')
+ output = Facter::Core::Execution.execute('php -v')

  unless output.nil?
(...)
>  Error: Facter: Error while resolving custom fact fact='phpversion', resolution='<anonymous>': Could not execute 'php -v': command not found

The trigger is obvious: the php module was being used on nodes that have no php. I pulled in the master branch in my Puppetfile, the module is pinned back to v12.0.0 there for now and that's sufficient for me - but others will for sure get into trouble later if this change is pushed out in an upcoming release.

Below the line is the AI-generated text saying more or less the same, just with more technical details.

f351625 (#767) introduced a regression for every node that does not have php
installed. Facter::Util::Resolution.exec returned nil when the command
could not be run; Facter::Core::Execution.execute raises
Facter::Core::Execution::ExecutionFailure instead. Facter catches the
exception, so the fact still resolves to nil, but it logs an error on every
run of every php-less node:

Error: Facter: Error while resolving custom fact fact='phpversion', resolution='<anonymous>': Could not execute 'php -v': command not found

That is the same class of noise that the nil guard in 6c1e69e was added to
prevent, for #134.

Passing on_fail: nil is the direct equivalent of the old exec semantics - it
is literally what the deprecated Facter::Core::Execution.exec did internally,
and what Facter's own string-form setcode does.

The fact resolves to nil with and without the fix, since Facter catches the
exception, so the added spec asserts the symptom instead: that nothing is
logged. It fails on master and passes with the one-line change.

Unrelated to this regression, and left alone here: a php that runs but writes
nothing to stdout still ends in NoMethodError on nil.split, because the
unless output.nil? guard lets "" through. Say the word and I will send a
separate patch.

This Pull Request (PR) fixes the following issues

Issue #134 - it was fixed previously, but the latest changes in the master branch reintroduce it.

🤖 Generated with Claude Code

Facter::Core::Execution.execute raises ExecutionFailure when the command cannot be run, whereas the Facter::Util::Resolution.exec it replaced in f351625 returned nil. Since that commit every node without php logs Error: Facter: Error while resolving custom fact fact='phpversion', resolution='<anonymous>': Could not execute 'php -v': command not found on every run - the same class of noise the nil guard in 6c1e69e (issue 134) was added to prevent. Passing on_fail: nil restores the old semantics, and is what the deprecated Facter::Core::Execution.exec did internally. The fact resolves to nil either way, since Facter catches and logs the exception, so the added spec asserts that nothing is logged. Assisted-By: Claude Opus 5 <noreply@anthropic.com> Reviewed-by: Tobias Brox <tobias@redpill-linpro.com>

kenyon

I think it should be using a confine like this: https://github.com/voxpupuli/puppet-letsencrypt/blob/836d38e5526e4b6286d3bfe28f1cd412ee7e2b4e/lib/facter/certbot_version.rb

Addresses a review comment on voxpupuli#768: a confine is the idiomatic way to keep a command-backed fact quiet where the command is missing, as done in puppet-letsencrypt's certbot_version fact. The fact is now not resolved at all on nodes without php, rather than running php -v and discarding the failure. The on_fail: nil from the previous commit is kept: it still covers a php that is in PATH but cannot run (broken install, missing libraries, permissions), which the confine does not catch. Prompt: (review comment on the pull request) I think it should be using a `confine` like this: https://github.com/voxpupuli/puppet-letsencrypt/blob/836d38e5526e4b6286d3bfe28f1cd412ee7e2b4e/lib/facter/certbot_version.rb Prompt: voxpupuli#768 got a comment. I believe the work on this was done under [a local working copy] Assisted-By: Claude Opus 5 <noreply@anthropic.com>

Addresses a review comment on voxpupuli#768: a confine is the idiomatic way to keep a command-backed fact quiet where the command is missing, as done in puppet-letsencrypt's certbot_version fact. The fact is now not resolved at all on nodes without php, rather than running php -v and discarding the failure. The on_fail: nil from the previous commit is kept: it still covers a php that is in PATH but cannot run (broken install, missing libraries, permissions), which the confine does not catch. Prompt: (review comment on the pull request) I think it should be using a `confine` like this: https://github.com/voxpupuli/puppet-letsencrypt/blob/836d38e5526e4b6286d3bfe28f1cd412ee7e2b4e/lib/facter/certbot_version.rb Assisted-By: Claude Opus 5 <noreply@anthropic.com>

Addresses a review comment on voxpupuli#768: a confine is the idiomatic way to keep a command-backed fact quiet where the command is missing, as done in puppet-letsencrypt's certbot_version fact. The fact is no longer resolved at all on nodes without php, so no php -v runs and nothing is logged - which is what issue voxpupuli#134 asked for. This also drops the on_fail: nil from the previous commit. A php in PATH that cannot run is a broken node rather than a php-less one, and an error in the log is the appropriate outcome there. Prompt: (review comment on the pull request) I think it should be using a `confine` like this: https://github.com/voxpupuli/puppet-letsencrypt/blob/836d38e5526e4b6286d3bfe28f1cd412ee7e2b4e/lib/facter/certbot_version.rb Followup-Prompt: If the php-command exists, but fails to run, probably it's appropriate with an error? Assisted-By: Claude Opus 5 <noreply@anthropic.com>

tobixen · 2026-09-18T05:53:10Z

Fixed

bastelfreak · 2026-09-18T07:15:47Z

+require 'spec_helper'
+
+describe 'phpversion fact' do
+  subject(:fact) { Facter.fact(:phpversion).value }


Is this actually executing facter? I don't think this is right. Same for the load in line 10. Please compare this to other tests, like https://github.com/voxpupuli/puppet-collectd/blob/master/spec/unit/collectd_real_version_spec.rb

Addresses a review comment on voxpupuli#768. The manual load of lib/facter/phpversion.rb is unnecessary: Facter.clear rebuilds the collection and its loader, and rspec-core puts the module's lib directory on $LOAD_PATH, where that loader looks. The spec now follows puppet-collectd's collectd_real_version_spec.rb: a type: :fact describe block, clear before and after, one example per case, stubs inline. Coverage is unchanged - reverting the fact to its pre-confine state still fails the "not defined if php is not installed" example. Prompt: (a comment in the pull request) Is this actually executing facter? I don't think this is right. Same for the `load` in line 10. Please compare this to other tests, like https://github.com/voxpupuli/puppet-collectd/blob/master/spec/unit/collectd_real_version_spec.rb Assisted-By: Claude Opus 5 <noreply@anthropic.com> Reviewed-by: Tobias Brox <tobias@redpill-linpro.com>

Dropping on_fail: nil left the `unless output.nil?` guard from 6c1e69e dead: Facter::Core::Execution.execute either raises or returns a String. The nil that can still occur is the first line of an empty output, and `nil.split` raised NoMethodError - logged by Facter on a node where php is installed and runs, which is the same noise issue voxpupuli#134 asked to stop. Also adds the example for the deliberate behaviour change in the previous commit: php in PATH but unrunnable now raises and is logged, which nothing asserted after that commit removed the example saying the opposite. Prompt: [review and push] Assisted-By: Claude Opus 5 <noreply@anthropic.com> Reviewed-by: Tobias Brox <tobias@redpill-linpro.com>

tobixen marked this pull request as ready for review September 17, 2026 23:34

kenyon reviewed Sep 18, 2026

View reviewed changes

tobixen force-pushed the phpversion-fact-no-raise branch from c94bb84 to 9d5a7a1 Compare September 18, 2026 05:41

tobixen force-pushed the phpversion-fact-no-raise branch from 9d5a7a1 to 9f3080c Compare September 18, 2026 05:51

kenyon approved these changes Sep 18, 2026

View reviewed changes

kenyon added the bug Something isn't working label Sep 18, 2026

bastelfreak reviewed Sep 18, 2026

View reviewed changes

tobixen added 2 commits September 18, 2026 12:33

tobixen mentioned this pull request Sep 18, 2026

phpversion fact may come out empty #769

Open

Navigation Menu

Uh oh!

Stop the phpversion fact raising without php - #768

Stop the phpversion fact raising without php#768
tobixen wants to merge 4 commits into
voxpupuli:masterfrom
tobixen:phpversion-fact-no-raise

tobixen commented Sep 17, 2026 •

edited

Loading

Uh oh!

kenyon left a comment

Uh oh!

tobixen commented Sep 18, 2026

Uh oh!

bastelfreak Sep 18, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tobixen commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Uh oh!

kenyon left a comment

Choose a reason for hiding this comment

Uh oh!

tobixen commented Sep 18, 2026

Uh oh!

bastelfreak Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tobixen commented Sep 17, 2026 •

edited

Loading