Skip to content

Remove Guzzle as dependency #16

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: main
Choose a base branch
from

Conversation

Kekos
Copy link

@Kekos Kekos commented May 25, 2025

The usage of the Guzzle lib inside this library was minimal, only used for URI parsing. Since I don't use Guzzle in my own projects, I thought it could be replaced in this package.

@SamMousa
Copy link
Contributor

Parse_url does not support relative urls.

Caution
This function may not give correct results for relative or invalid URLs, and the results may not even match common behavior of HTTP clients. If URLs from untrusted input need to be parsed, extra validation is required, e.g. by using filter_var() with the FILTER_VALIDATE_URL filter.

@Kekos
Copy link
Author

Kekos commented May 25, 2025

Yes, I can see that being a problem. I'll check on it some time.

Can I run the static analysis locally? Could only find PHPUnit as a dependency.

@SamMousa
Copy link
Contributor

Yes, just get phpstan. For example using phive.

@Kekos Kekos force-pushed the replace-guzzle branch from bc77813 to b61d827 Compare May 26, 2025 20:51
@Kekos
Copy link
Author

Kekos commented May 26, 2025

I'm not quite sure how a relative URL would apply to the use cases of the \Codeception\Util\Uri metods?

@SamMousa
Copy link
Contributor

Don't force push please. It makes reviewing harder!

@SamMousa
Copy link
Contributor

I'm not quite sure how a relative URL would apply to the use cases of the \Codeception\Util\Uri metods?

From the webdriver module:

private function filterNodesByHref(string $url, array $nodes): array
    {
        //current uri can be relative, merging it with configured base url gives absolute url
        $absoluteCurrentUrl = Uri::mergeUrls($this->_getUrl(), $this->_getCurrentUri());
        $expectedUrl = Uri::mergeUrls($absoluteCurrentUrl, $url);
        return array_filter(
            $nodes,
            function (WebDriverElement $e) use ($expectedUrl, $absoluteCurrentUrl): bool {
                $elementHref = Uri::mergeUrls($absoluteCurrentUrl, $e->getAttribute('href') ?? '');
                return $elementHref === $expectedUrl;
            }
        );
    }

@Kekos
Copy link
Author

Kekos commented Jun 1, 2025

Thank you! I've checked the use-cases from the webdriver snippet and those seem to be covered by the unit tests.

The comment for $absoluteCurrentUrl tells that the "current uri" might be relative, it goes in the 2nd argument to mergeUrls(), which was never parsed by Guzzle code anyway.

But I must say, I'm very new to Codeception and it's highly possible I'm missing some very important context. Either way, I'm leaning towards using the PHAR version of Codeception instead.

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.

2 participants