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

Add Windows support for creating search adapters #452

Merged
merged 2 commits into from
Nov 3, 2024

Conversation

zoglo
Copy link
Contributor

@zoglo zoglo commented Oct 23, 2024

Another try but now in the right repo :)

Description

Absolute paths on windows fail due to an undefined array key within the following line not having a scheme at all;
https://github.com/schranz-search/schranz-search/blob/d91446a972b7bcc894abfd803eff40fc7dd3c09c/packages/seal/src/Adapter/AdapterFactory.php#L140

This PR adds Windows support for the path resolution since $dsn would fail later due to the second parse_url attempt returning false again.

In my case, the following $dsn would fail because it won't detect any scheme at all:
loupe://C:\path\project\var\indexes
The early return is done before adding the fallback where it adds the adapter name with an `@´ :')

Unsure if we should go for the PHP_OS_FAMILY check and see if it's windows in this case 🤔?

@alexander-schranz alexander-schranz added the bug Something isn't working label Oct 23, 2024
@zoglo
Copy link
Contributor Author

zoglo commented Oct 26, 2024

See 085fc7e
I left the explanation here: #452 (comment)

alexander-schranz added a commit that referenced this pull request Nov 3, 2024
To make #452 better
testable I added a few tests to for the AdapterFactory parseDsn part.
alexander-schranz added a commit to PHP-CMSIG/seal that referenced this pull request Nov 3, 2024
To make PHP-CMSIG/search#452 better
testable I added a few tests to for the AdapterFactory parseDsn part.
@alexander-schranz
Copy link
Member

alexander-schranz commented Nov 3, 2024

@zoglo did refactor it to rawurlencode the part and decode it later, as I stumbled over other issues in the later process. I'm not sure about the detection based on DIRECTORY_PATH or OS as may the "build" system for containers may a different as where being hosted. So I did manually detection by check if it starts with <letter>:\ this also let us test it in the Linux CI. With the new AdapterFactoryTest it should now be a lot easier to add more tests cases and avoid regressions here.

Thx for your work on this. Let me know if you stumble over more issues.

@alexander-schranz alexander-schranz changed the title Windows support for creating search adapters Add Windows support for creating search adapters Nov 3, 2024
@alexander-schranz alexander-schranz merged commit 4989ab5 into PHP-CMSIG:0.6 Nov 3, 2024
32 checks passed
@alexander-schranz alexander-schranz added this to the 1.0.0 milestone Nov 3, 2024
@zoglo zoglo deleted the fix/windows branch November 4, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants