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

Autoload relative paths first to avoid confusion with files from the global include path #2751

Merged
merged 1 commit into from
Dec 8, 2019
Merged

Conversation

klausi
Copy link
Contributor

@klausi klausi commented Dec 7, 2019

Problem: autoload files are confused with globally available files. See pfrenssen/coder#54

If you specify <autoload>./autoload.php</autoload> in your ruleset.xml file then a completely different autoload.php file from your system might be executed, even although you specified a relative path.

Steps to reproduce:

  1. Clone https://github.com/klausi/test_phpcs
  2. Run composer install
  3. Run ./vendor/bin/phpcs --standard=./standard test.php

Output:
PHP Parse error: syntax error, unexpected 'invalid' (T_STRING) in test_phpcs/autoload.php on line 3

Which means the wrong autoload.php file was included.

Proposed solution: Try to include relative paths first, then try global paths.

Not sure how we can test this, any tips for that?

@klausi
Copy link
Contributor Author

klausi commented Dec 7, 2019

Workaround we deployed for Coder: pfrenssen/coder#72 . We are just using a more unique autoload file name in the hopes that it does not get confused with other PHP files on the user's system.

Copy link
Contributor

@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.

Haven't tested this yet, but this change looks sane to me.

@gsherwood gsherwood added this to the 3.5.4 milestone Dec 8, 2019
// Try relative autoload paths first.
$relativePath = Util\Common::realPath(dirname($rulesetPath).DIRECTORY_SEPARATOR.$autoloadPath);

if ($relativePath !== false && is_file($$relativePath) === true) {
Copy link
Member

Choose a reason for hiding this comment

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

Double $ needs to be removed here. I'll do this while merging.

@gsherwood gsherwood changed the title fix(ruleset): Autoload relative paths correctly to avoid confusion with files from the global include path Autoload relative paths first to avoid confusion with files from the global include path Dec 8, 2019
gsherwood added a commit that referenced this pull request Dec 8, 2019
@gsherwood gsherwood merged commit 5e124a8 into squizlabs:master Dec 8, 2019
@gsherwood
Copy link
Member

Thanks a lot for this fix.

@klausi
Copy link
Contributor Author

klausi commented Dec 9, 2019

Ah, very sorry for the double $. That's what I get for making last minute changes.

Let me know If there is a good way to write a test for this.

@gsherwood
Copy link
Member

Ah, very sorry for the double $. That's what I get for making last minute changes.

Not a problem at all :)

Let me know If there is a good way to write a test for this.

I don't have one right now.

@klausi klausi deleted the autoload-relative branch April 14, 2023 08:51
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.

3 participants