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

chore: Optimize the usage of option 'baseURI' for CURLRequest #9274

Closed
wants to merge 2 commits into from

Conversation

warmbook
Copy link

#9265
Description
Use option 'baseURI' first in Config\Services->curlrequest.
Use the property 'uri' of OutgoingRequest as default baseURI instead of the property 'baseURI' of CURLRequest.
Remove the property 'baseURI' of CURLRequest.
Nolonger parse 'baseURI' in 'parseOptions' method.
If 'baseURI' exists in 'request' method's options, create a new URI instance with it. Otherwise, use the property 'uri' of OutgoingRequest.
Use the param '$uri' passed in rather than the property 'baseURI' of CURLRequest in 'prepareURL' method.
Remove 'getBaseURI' method from Test/Mock/MockCURLRequest.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@warmbook warmbook closed this Nov 13, 2024
@warmbook warmbook reopened this Nov 13, 2024
@warmbook warmbook changed the base branch from 4.6 to develop November 13, 2024 06:18
@michalsn
Copy link
Member

Just like in your previous PR: removing properties from the class or adding required parameters to the existing methods is considered a BC break.

#9273 (comment)

@michalsn michalsn added the breaking change Pull requests that may break existing functionalities label Nov 13, 2024
@warmbook
Copy link
Author

@michalsn Emmm... Now that 'there is no need to worry about deprecating anything',why could not us deprecate the property 'baseURI' of CURLRequest?

@michalsn
Copy link
Member

@warmbook I was talking about base_uri. The use of base_uri is not documented anywhere, and the switch to camelCase happened before the official v4 release. The use of base_uri is a bug. There is no point in deprecating the bug.

In your code, you're simply removing things without a good reason, which is not acceptable.

Copy link
Contributor

@neznaika0 neznaika0 left a comment

Choose a reason for hiding this comment

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

If you wanted to fix it, all you need to do is replace $options['baseURI'] ?? null

@warmbook
Copy link
Author

Thanks! I've got it. But how could I submit the left suggestions? I mean, I have some idea about this framework apart from fixing this bug. @michalsn @neznaika0

@michalsn
Copy link
Member

Enhancements and new features can be sent to the 4.6 branch. The develop branch is for bug fixes only.

Just remember that any breaking changes must have a really good reason to be accepted. Backward compatibility is preferred.

@warmbook
Copy link
Author

Thanks! I've got it. @michalsn

@michalsn michalsn added the GPG-Signing needed Pull requests that need GPG-Signing label Nov 14, 2024
@warmbook warmbook closed this by deleting the head repository Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities GPG-Signing needed Pull requests that need GPG-Signing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants