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.

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

Automatically detect PHP extension directory #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ddeboer
Copy link

@ddeboer ddeboer commented Jul 4, 2013

When adding, for instance, the XDebug module, one currently has to write:

php::module::ini { 'pecl-xdebug':
  zend => '/usr/lib64/php/modules',
}

With this PR it becomes easier to include Zend extensions like XDebug:

php::module::ini { 'pecl-xdebug':
  zend => true,
}

In this case, the PHP extension directory is automatically detected through a Puppet fact. If you need to override the module directory, you can still do so by supplying a string for the zend property:

php::module::ini { 'pecl-xdebug':
  zend => '/some/other/php/modules/dir',
}

@thias
Copy link
Owner

thias commented Jul 4, 2013

This is an interesting idea. Unfortunately, the php-config command is provided by the php-devel package on RHEL, meaning that many servers won't have it available (mine don't, I keep devel packages off of production systems).

And the default you set if the fact isn't present is 64bit specific, which is something best avoided.

How would you make the fact more robust? Fall back to iterating through known directories until one is found, when the php-config command is unavailable?

@ddeboer
Copy link
Author

ddeboer commented Jul 4, 2013

Good points. I made the fact architecture-aware and independent from php-devel.

Of course, the most reliable output is from php -i itself, but that will not be available in the Puppet run during which PHP is installed (only in subsequent runs). So I guess we do need to iterate through a set of known directories. Any suggestions as to which directories they should be?

@thias
Copy link
Owner

thias commented Apr 1, 2014

Sorry for taking soooooooooooo long. I'm still not entirely convinced about this change, but I would be okay to include it with a minor clean up : There currently are two separate fallbacks, one in the fact and the other in params.pp. The one in params.pp is uglier since it harcodes the lib64 path, so it will be incorrect on 32bit systems. The one is the fact is less useful, as it's nice to have a fallback if pluginsync is disabled (no fact)... so... if you can clean that up, I'll include the change.

I think just removing the fallback in params.pp would be fine, since people passing true will be doing so explicitly.

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.

2 participants