Issue #83: Refactor wopi controller #84
Conversation
donquixote
commented
Jan 8, 2025
•
donquixote
commented
- Fixes Refactor WopiController #83
- Depends on Issue #75: WOPI proof #79
… in services.yml, and setting Content-Type header. Without that, the ExceptionWopiSubscriber was not actually registered as an event subscriber. It did not matter because routes with any '_format' other than 'html' already behave in the desired way. Now, to replicate the prior behavior, we must explicitly set the 'Content-Type' header to 'text/plain'.
…t keep 403. Changes: - Use wildcard loading for media entity in WopiController. - Change 'not found' response code and message for WOPI requests, to replicate old behavior. Subsequent commit will change to actual 404.
It is not needed.
donquixote
force-pushed
the
issue-83-refactor-wopi-controller
branch
from
628315d to
fa01d4c
Compare
donquixote
force-pushed
the
issue-83-refactor-wopi-controller
branch
from
fa01d4c to
23836ab
Compare
donquixote
force-pushed
the
issue-83-refactor-wopi-controller
branch
from
1883815 to
0a91f3a
Compare
| @@ -175,7 +175,7 @@ protected function getSubject(Request $request): string { | |||
| pack('N', strlen($url)), | |||
| strtoupper($url), | |||
| pack('N', 8), | |||
| pack('J', $timestamp_ticks) | |||
| pack('J', $timestamp_ticks), | |||
There was a problem hiding this comment.
phpcs missed this?
|
|
||
| if ($wopi_stamp != $file_stamp) { | ||
| $this->logger->error('Conflict saving file ' . $id . ' wopi: ' . $wopi_stamp->format('c') . ' differs from file: ' . $file_stamp->format('c')); | ||
| $this->logger->error('Save reason: ' . $save_reason); |
There was a problem hiding this comment.
Should we log an error always?
| @@ -342,12 +364,12 @@ protected function verifyTokenForMediaId( | |||
| if ($values === NULL) { | |||
There was a problem hiding this comment.
Line: 361 Typo in the context message variable name
| * @return string | ||
| * Formatted date string in ISO 8601 format, in UTC. | ||
| */ | ||
| public static function format(int $timestamp): string { |
There was a problem hiding this comment.
Format as function name seems a bit generic.
There was a problem hiding this comment.
Should we add license here? Other files may need it too.
| ], | ||
| $request, | ||
| ); | ||
| $this->assertTrue($this->logger->hasRecord('Save reason: ' . $reason_message)); |
There was a problem hiding this comment.
I think we should check the type of record too.
| * This is expected to be an array. | ||
| */ | ||
| protected function assertPartialArray(array $expected, mixed $actual): void { | ||
| $this->assertIsArray($actual); |
There was a problem hiding this comment.
Why mixed type and not require an array as parameter?
| // The file name is preserved. | ||
| $this->assertSame('test.txt', $new_file->getFilename()); | ||
| // The file owner is preserved. | ||
| $this->assertSame($this->fileOwner->id(), $new_file->getOwnerId()); |
There was a problem hiding this comment.
Maybe is good to add a check to see that it is permanent.
| $i = $this->getCounterValue(); | ||
| // The request time is always the same. | ||
| $file_changed_time = \DateTimeImmutable::createFromFormat('U', (string) $this->file->getChangedTime()); | ||
| $new_file_content = "File content $i."; |
There was a problem hiding this comment.
What about an incremental content to see that size changes?
There was a problem hiding this comment.
I miss some tests or checks:
- Test with missing access_token. Message: 'Missing access token.'
- Test with no file attached to media. Message: 'No file attached to media.'
- Test token that can't be decoded: Message: 'Malformed token.'
- Check Info response with user picture.
There is also the case where the $action doesn't exist in wopi() but not sure if can be easily tested, and same for $can_write = FALSE in wopiPutFile().
