Skip to content

Config: remove code related to the now unsupported PEAR installation #1102

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rodrigoprimo
Copy link
Contributor

@rodrigoprimo rodrigoprimo commented May 22, 2025

Description

PR #4 dropped support for installing PHPCS via PEAR. This commit removes code related to a PEAR placeholder called @data_dir@ as it is not necessary anymore.

The original version of this code was added in 980c835#diff-36ecedca179eab0b3cd245e872a96ab26fa08e567437e167f4eda1779c15c89R1370-R1372. Two methods were added that use the @data_dir@ placeholder in that commit: setConfigData() and getAllConfigData(). A subsequent commit changed setConfigData() to only use the PEAR path for the config file if the @data_dir@ placeholder was replaced with something else: 7bb7383. A later commit did the same for getAllConfigData() together with other unrelated changes: 9392185.

It seems that the PEAR file package.xml was responsible for replacing @data_dir@ with an actual path:

PHP_CodeSniffer/package.xml

Lines 293 to 295 in 6d22f38

<file baseinstalldir="PHP/CodeSniffer" name="Config.php" role="php">
<tasks:replace from="@data_dir@" to="data_dir" type="pear-config" />
</file>
.

PR #4 removed the @data_dir@ related code from setConfigData(), but not from getAllConfigData().

If we want to play it safe (if there are any doubts), the PR should be pulled to 4.x. Otherwise it can go into 3.x.

Regarding this question asked in the issue, from all that I can see, it is safe to remove this code in 3.x. The removed if condition only evaluates to true and thus changes the value of the $configFile variable if the @data_dir@ placeholder is modified in the code, which should not happen under supported circumstances. That being said, I'm happy to change the base branch of this PR to 4.x if you prefer.

Suggested changelog entry

N/A

Related issues/external references

Fixes #1101

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

PR 4 dropped support for installing PHPCS via PEAR. This commit removes
code related to a PEAR placeholder called `@data_dir@` as it is not
necessary anymore.

The original version of this code was added in PHPCSStandards@980c835#diff-c36ecedca179eab0b3cd245e872a96ab26fa08e567437e167f4eda1779c15c89R1370-R1372.
Two methods were added that use the `@data_dir@` placeholder in that
commit: `setConfigData()` and `getAllConfigData()`. A subsequent commit
changed `setConfigData()` to only use the PEAR path for the config file
if the `@data_dir@` placeholder was replaced with something else:
PHPCSStandards@7bb7383.
A later commit did the same for `getAllConfigData()` together with other
unrelated changes: PHPCSStandards@9392185.

It seems that the PEAR file package.xml was responsible for replacing
`@data_dir@` with an actual path: https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/6d22f38f6c846512d321f3d5ac15c8805c694084/package.xml#L293-L295.

PR 4 removed the `@data_dir@` related code from `setConfigData()`, but
not from `getAllConfigData()`.

Closes 1101
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing that backtracing to verify when & why this was originally added @rodrigoprimo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential PEAR installation related code block found
2 participants