Skip to content

Replace file path when it is encountered in string constants #410

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

Conversation

asgrim
Copy link
Member

@asgrim asgrim commented Jan 15, 2022

Fixes #400

@asgrim asgrim added the bug label Jan 15, 2022
return $value;
}

return str_replace(dirname(realpath($filePath)), self::MAGIC_DIR_OR_FILE_VALUE, $value);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not 100% convinced this is "the best way":tm: but it was just an idea I put together. Thoughts welcome!

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, looks iffy

@asgrim asgrim force-pushed the 400-replace-file-path-when-path-encountered-in-string-constants branch from da0a2c3 to 0c9330e Compare January 15, 2022 10:52
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

After thinking further about it, I think the correct way would be to change the path detected for source locators, which would fix a whole category of issues related to this.

I don't know yet how to approach this, but shadowing the /tmp location completely seems like a better approach.

@Ocramius
Copy link
Member

One approach I thought of (not sure if viable), would be to replace LocatedSource paths via decoration:

return new LocatedSource(Filesystem\read_file($classFile), $identifier->getName(), $classFile);

We could produce a VirtualPathLocatedSource or such, where LocatedSource's path getter is decorated to remove the TMP path section 🤔

@asgrim asgrim self-assigned this Jan 29, 2022
@Ocramius
Copy link
Member

Ocramius commented Feb 8, 2022

I've potentially identified a way of making this work in a relatively hacky, but clean way.

The way we find symbols via source locators is via Roave\BetterReflection\SourceLocator\Ast\Locator.

While Roave\BetterReflection\SourceLocator\Ast\Locator is marked @internal, it is a core component to source locators, and its job is to convert string sources to reflection symbols.

We could decorate the Locator, and make sure that any LocatedSource instance passed to it is decorated, so that LocatedSource#getFileName() is stripped of any temporary source path.

That would make __DIR__ and __FILE__ work as expected again, I think.

@asgrim
Copy link
Member Author

asgrim commented Feb 8, 2022

@Ocramius that sounds like a plausible approach; will try and look into this when I can

@Ocramius Ocramius changed the base branch from 6.1.x to 6.3.x March 8, 2022 16:12
Ocramius added a commit that referenced this pull request Mar 8, 2022
…the base path of `LocatedSource` instances

This fails due to `CompileNodeToValue()` internally using `realpath()` and `dirname()` operations, which both depend
on the current filesystem: as it currently stands, this requires an improvement of `roave/better-reflection` first.

Also, `ReplaceSourcePathOfLocatedSources` and `LocatedSourceWithStrippedSourcesDirectory` are extending `@internal`
symbols, so we may want to first make these symbols public in `roave/better-reflection`, to avoid the BC boundary to
become too relaxed, and therefore causing crashes if we (rightfully) decide to change `roave/better-reflection` internals.

Fixes #400
Fixes #410
@Ocramius
Copy link
Member

Ocramius commented Mar 8, 2022

I sent up a patch with my proposed approach @ #476

It will need patching roave/better-reflection to get rid of realpath() and dirname() from its code, but unsure if that's feasible at all.

Ocramius added a commit that referenced this pull request Mar 22, 2022
…the base path of `LocatedSource` instances

This fails due to `CompileNodeToValue()` internally using `realpath()` and `dirname()` operations, which both depend
on the current filesystem: as it currently stands, this requires an improvement of `roave/better-reflection` first.

Also, `ReplaceSourcePathOfLocatedSources` and `LocatedSourceWithStrippedSourcesDirectory` are extending `@internal`
symbols, so we may want to first make these symbols public in `roave/better-reflection`, to avoid the BC boundary to
become too relaxed, and therefore causing crashes if we (rightfully) decide to change `roave/better-reflection` internals.

Fixes #400
Fixes #410
@Ocramius Ocramius added this to the 6.3.0 milestone Mar 22, 2022
@Ocramius
Copy link
Member

Closing this one: approach in #476 works fine, and is a bit better than operating on the constant expressions, as those can really occur almost anywhere in the system, so relying on the abstractions by roave/better-reflection is indeed to be prioritized.

@Ocramius Ocramius closed this Mar 22, 2022
@Ocramius Ocramius deleted the 400-replace-file-path-when-path-encountered-in-string-constants branch March 22, 2022 17:50
Ocramius added a commit that referenced this pull request Mar 23, 2022
…hs-to-prevent-__DIR__-and-__FILE__-bc-break-false-positives

Normalize `__DIR__` and `__FILE__` by changing the base path of `LocatedSource` instances, prevent false-positive BC breaks on those magic constants
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.

False positive result when __DIR__ is used
2 participants