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

添加自定义ScopingHttpClient降低使用复杂度 #2766

Merged
merged 3 commits into from Dec 14, 2023

Conversation

hughcube
Copy link
Contributor

@hughcube hughcube commented Dec 7, 2023

No description provided.

Copy link

vercel bot commented Dec 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
easywechat ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 12, 2023 4:08am

public function request(string $method, string $url, array $options = []): ResponseInterface
{
foreach ($this->defaultOptionsByRegexp as $regexp => $defaultOptions) {
if (preg_match("{{$regexp}}A", $url)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里的 A 是?

@hughcube
Copy link
Contributor Author

@overtrue 超哥, 这个是一个修饰符 PCRE_ANCHORED

A (PCRE_ANCHORED)
如果设置了这个修饰符,模式被强制为"锚定"模式,也就是说约束匹配使其仅从 目标字符串的开始位置搜索。这个效果同样可以使用适当的模式构造出来,并且 这也是 perl 种实现这种模式的唯一途径。

@TheNorthMemory
Copy link

foreach循环里使用preg_match,而且每次是从字符串开始位替换,效率应该不会很好。

"{{$regexp}}A" 这个可能也会有一些不太适应的地方,例如 $regexp = /[^/]+/; PHP则会翻译成 "{/[^/]+/}A",这可能并非设计目标,建议增加测试用例覆盖或者注释说明怎样去构造这个 $regexp 变量。

PS: 都已经设计成 模式匹配方式了,为啥不考虑把 "A" modifier 也声明进模式变量内呢,万一有诉求说:”期望顺序最多匹配到一次“,那这个 "A" 就制造麻烦了。

@hughcube
Copy link
Contributor Author

hughcube commented Dec 12, 2023

@TheNorthMemory
性能问题倒是还好, 实际运行应该差距很少.
Symfony 的代码, 他推荐的是对一个完整的url 进行验证, 实际操作中也是会推荐这样.

模式放进 $regexp 也不是不可以, 实际上大部分的使用场景都不会关心这个

已经去掉了A模式, 现在应该没啥争议了

@overtrue overtrue merged commit 557409d into w7corp:6.x Dec 14, 2023
6 of 9 checks passed
@hughcube hughcube deleted the patch-2 branch December 14, 2023 04:34
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.

None yet

3 participants