Issue #56: Refactor CoolRequest #68
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
donquixote
force-pushed
the
issue-56-refactor-CoolRequest
branch
from
9c78e89 to
bd16013
Compare
donquixote
force-pushed
the
issue-56-refactor-CoolRequest
branch
2 times, most recently
from
a88a469 to
6b73cbb
Compare
donquixote
commented
Nov 28, 2024
src/Cool/CollaboraConnection.php
Outdated
| "The configured Collabora Online server address must begin with 'http://' or 'https://'. Found '%s'.", | ||
| $wopi_client_server, | ||
| )); | ||
| } |
There was a problem hiding this comment.
All this validation of config values seems a bit out of place.
In most of Drupal core, config values are simply trusted - for better or worse.
But, currently the config form itself does not really validate these settings.
We do have a schema, but in the config form I can just enter any value I like for the server and host url, e.g. "abc".
Imo we should keep the validation as-is for now, and only change it once we have proper validation in the form.
(The validation is coming from the original code, it has been moved around and refactored but it is still mostly equivalent with the original code.)
Original steps: - Rename vars in strStartsWith(). - Simplify strStartsWith(). - Fix strStartsWith() by using strpos() instead of strrpos(). - Replace strStartsWith() with str_starts_with().
It can happen that file_get_contents() hangs at the end of a stream, waiting for more data. With curl it seems this does not happen. Even better would be to use e.g. Guzzle http client, but that's too big a change for now.
Changes: - Narrow parameter type for $discovery_parsed, don't check for NULL or FALSE anymore. - Use (string) cast instead of strval(). - Harden the condition for return value from xpath. - Let getWopiSrcUrl() always return string or throw exception. - Inline the function. - Invert if statement, to align with the rest.
donquixote
force-pushed
the
issue-56-refactor-CoolRequest
branch
from
6b73cbb to
840ead8
Compare
AaronGilMartinez
approved these changes
Dec 2, 2024
donquixote
force-pushed
the
issue-56-refactor-CoolRequest
branch
from
840ead8 to
c40af62
Compare
…t in ViewerController. Also make the private property protected.
…e value to getViewerRender().
donquixote
force-pushed
the
issue-56-refactor-CoolRequest
branch
2 times, most recently
from
143cce9 to
7ebfa5c
Compare
donquixote
force-pushed
the
issue-56-refactor-CoolRequest
branch
from
7ebfa5c to
e663b8d
Compare
AaronGilMartinez
approved these changes
Dec 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Further changes will be done in a follow-up issue.