Skip to content

Navigator: Fix attempt to create existing destination#2776

Merged
dantleech merged 8 commits intophpactor:masterfrom
bart-jaskulski:gh-2758
Nov 18, 2024
Merged

Navigator: Fix attempt to create existing destination#2776
dantleech merged 8 commits intophpactor:masterfrom
bart-jaskulski:gh-2758

Conversation

@bart-jaskulski
Copy link
Contributor

@bart-jaskulski bart-jaskulski commented Nov 14, 2024

Remove false-positive file creation attempt by resolving absolute path of destination. Introduce a rough set of tests for that, for further refinement.

Fix #2758

FilePathResolverExtension::class,
WorseReflectionExtension::class,
], array_merge([
LoggingExtension::PARAM_ENABLED=> true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@dantleech dantleech Nov 17, 2024

Choose a reason for hiding this comment

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

because the COMPOSER_AUTOLOADER path is based on the APPLICATION_ROOT perhaps?

The reason these integration test cases all over the place is because these were originally separate packages. At some level it would make sense to have a single "Phpactor" IntegrationTestCase providing the workspace + container, or maybe a better way to bootstrap it but it's not a trivial thing to do at this scale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In IntegrationTestCase I disabled composer extension

@dantleech
Copy link
Collaborator

can you summarise what the problem was and how this fixes it?

@bart-jaskulski
Copy link
Contributor Author

Using RPC command to navigate between files by pattern (like go to test/go to source) with file autocreation method enabled always resulted in a prompt asking for creating a target file, even though it might already exist.

This happened, because destination file was provided as path relative to current working directory (directly from user setting, like src/<kernel>.php), which somehow proved to be unresolvable in runtime. During tests I've found that cwd and workspace path may differ.

The fix simply resolves to absolute path to avoid any disambiguity. It is applied at the latest possible stage, affecting both checking for the need to create a file and the target destination delivered back from the RPC command.

@dantleech
Copy link
Collaborator

dantleech commented Nov 16, 2024

Are you using the PHAR? (I don't get this issue, but it sounds like a PHAR thing)

@bart-jaskulski
Copy link
Contributor Author

I don't get this issue

Look at the videos attached in #2758

And yes, it does appear when using phpactor built as PHAR. But the test suite somewhat confirms the issue (yet that may be another way of mangling paths).

Remove false-positive file creation attempt by resolving absolute path
of destination. Introduce rough set of tests for that, for further
refinement.

Signed-off-by: Bart Jaskulski <[email protected]>
Signed-off-by: Bart Jaskulski <[email protected]>
Signed-off-by: Bart Jaskulski <[email protected]>
Signed-off-by: Bart Jaskulski <[email protected]>
Signed-off-by: Bart Jaskulski <[email protected]>
@bart-jaskulski
Copy link
Contributor Author

failure in latest PHPUnit seems to be false-positive -- related to indexer and I don't experience it locally (even when running in PHP 8.2)

@dantleech dantleech merged commit cc28b51 into phpactor:master Nov 18, 2024
@dantleech
Copy link
Collaborator

yes, that test has been flakey for years, thanks :)

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.

Navigator was try generate already exists source\unit_test classes

2 participants