Skip to content

Conversation

@julien-nc
Copy link
Member

This makes a HEAD request before the GET one and skips links if either:

  • content-type is not text/html
  • or content-length is more than 5 MB

Another approach would be to use the stream Guzzle option when performing the GET request to avoid the extra HEAD request and either:

  1. stop reading the body after a size limit
    I don't know if it makes sense since we probably can't properly parse a partial html content.

  2. directly check the size and type from the headers and skip bad links even before reading the body.
    I don't know if the headers are retrieved before downloading/streaming the body.

What do you think?

… resolve with opengraph link provider

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

LGTM

@szaimen
Copy link
Contributor

szaimen commented Jan 5, 2023

Not sure where this comes from:

There was 1 failure: | 59s
-- | --
654 |   | 59s
655 | 1) OCA\Settings\Tests\Controller\CheckSetupControllerTest::testCheck | 59s
656 | Failed asserting that two objects are equal. | 59s
657 | --- Expected | 59s
658 | +++ Actual | 59s
659 | @@ @@ | 59s
660 | 'OCA\Settings\SetupChecks\PhpOutputBuffering' => Array (...) | 59s
661 | 'OCA\Settings\SetupChecks\LegacySSEKeyFormat' => Array (...) | 59s
662 | 'OCA\Settings\SetupChecks\CheckUserCertificates' => Array (...) | 59s
663 | -        'imageMagickLacksSVGSupport' => false | 59s
664 | +        'imageMagickLacksSVGSupport' => true

@julien-nc
Copy link
Member Author

@szaimen I don't know either but it's not related.

@julien-nc julien-nc enabled auto-merge January 5, 2023 14:26
@julien-nc
Copy link
Member Author

/backport to stable25

@juliusknorr
Copy link
Member

Failure is also there on master.

@szaimen szaimen merged commit a36848c into master Jan 5, 2023
@szaimen szaimen deleted the enh/noid/opengraph-link-reference-limits branch January 5, 2023 15:01
max-nextcloud added a commit to nextcloud/text that referenced this pull request Jan 8, 2023
* CI passes with Use latest server version that worked on master
* Try to revert nextcloud/server#36016

Signed-off-by: Max <max@nextcloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants