Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Classes/Controller/BrokenLinkListController.php
Original file line number Diff line number Diff line change
Expand Up @@ -1003,11 +1003,11 @@ protected function renderTableRow($table, array $row): array
htmlspecialchars($languageService->sL('LLL:EXT:brofix/Resources/Private/Language/Module/locallang.xlf:list.msg.status.excluded')) ?: 'URL is excluded, will not be checked'
);
break;
default:
// todo add language label
default: // This case will handle LinkTargetResponse::RESULT_UNKNOWN (which is 5)
// todo add language label for list.msg.status.cloudflare
$linkMessage = sprintf(
'<span class="status-unknown">%s</span>',
htmlspecialchars($languageService->sL('LLL:EXT:brofix/Resources/Private/Language/Module/locallang.xlf:list.msg.status.unknown')) ?: 'Unknown status'
'<span class="status-cloudflare">%s</span>', // Consider adding a specific CSS class if styling is needed
htmlspecialchars($languageService->sL('LLL:EXT:brofix/Resources/Private/Language/Module/locallang.xlf:list.msg.status.cloudflare')) ?: 'Cloudflare link'
);
break;
}
Expand Down
6 changes: 6 additions & 0 deletions Classes/LinkAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,12 @@ protected function checkLinks(array $links, array $linkTypes, int $mode = 0): vo
}
$this->debug("checkLinks: after checking $url");

// Check for Cloudflare
if ($linkTargetResponse->getReasonCannotCheck() == LinkTargetResponse::REASON_CANNOT_CHECK_CLOUDFLARE) {
$linkTargetResponse->setStatus(LinkTargetResponse::RESULT_UNKNOWN);
Copy link
Owner

Choose a reason for hiding this comment

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

I am not sure I understand what is the intention of this change.

I like the idea of the icon for cloudflare.

But I don't like the idea of introducing a new status RESULT_UNKNOWN for it. Currently, the status RESULT_CANNOT_CHECK is used, which I think makes sense: we cannot effectively currently check these kind of URLs.

Also, when I apply the patch, the behaviour does not change - it still shows this as before, which is currently intended behaviour:

image

In a previous change, the status was introduced which could not only be "broken" or "not broken" but also e.g. REASON_CANNOT_CHECK (as defined in LinkTargetResponse).

I already introduced code to detect Cloudflare. If correctly detected, this gets the status RESULT_CANNOT_CHECK and the reason REASON_CANNOT_CHECK_CLOUDFLARE.

I don't see an advantage in now introducing a new status RESULT_UNKNOWN.

$linkTargetResponse->setReasonCannotCheck(LinkTargetResponse::REASON_CANNOT_CHECK_CLOUDFLARE);
}

$this->statistics->incrementCountLinksByStatus($linkTargetResponse->getStatus());

// Broken link found
Expand Down
1 change: 1 addition & 0 deletions Documentation/Setup/ExtensionConfigurationReference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ Currently, these are the known status:
* 2: ok
* 3: not possible to check ("non-checkable")
* 4: is excluded
* 5: Cloudflare link

This should also improve handling of cloudflare protected sites as these
typically return 403 HTTP status code. The link checking status is no longer
Expand Down
6 changes: 6 additions & 0 deletions Resources/Private/Language/Module/locallang.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@
<trans-unit id="list.filter.check_status.4" resname="list.filter.check_status.4">
<source>excluded</source>
</trans-unit>
<trans-unit id="list.filter.check_status.5" resname="list.filter.check_status.5">
<source>Cloudflare</source>
</trans-unit>

<trans-unit id="list.recheck.links.title" resname="list.recheck.links.title">
<source>Recheck links</source>
Expand Down Expand Up @@ -410,6 +413,9 @@
<trans-unit id="list.no.broken.links.filter" resname="list.no.broken.links.filter">
<source>No broken links found with current filter!</source>
</trans-unit>
<trans-unit id="list.msg.status.cloudflare" resname="list.msg.status.cloudflare">
<source>Cloudflare link</source>
</trans-unit>
<trans-unit id="no.access.title" resname="no.access.title">
<source>No access!</source>
</trans-unit>
Expand Down
1 change: 1 addition & 0 deletions Resources/Private/Partials/BrokenLinkList.html
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@
<f:if condition="{item.status} === 2"><core:icon identifier="actions-check-circle"/></f:if>
<f:if condition="{item.status} === 3"><core:icon identifier="actions-question-circle"/></f:if>
<f:if condition="{item.status} === 4"><core:icon identifier="actions-ban"/></f:if>
<f:if condition="{item.status} === 5"><core:icon identifier="actions-cloud"/></f:if>
{item.linkmessage -> f:format.raw()}</td>

<f:comment>==== last_check_url ====</f:comment>
Expand Down
3 changes: 3 additions & 0 deletions Resources/Private/Partials/BrokenLinksForm.html
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@
<option value="4" {f:if(condition:'{filter.checkStatusFilter} == 4', then: 'selected')}>
<f:translate extensionName="Brofix" key="LLL:EXT:brofix/Resources/Private/Language/Module/locallang.xlf:list.filter.check_status.4"/>
</option>
<option value="5" {f:if(condition:'{filter.checkStatusFilter} == 5', then: 'selected')}>
<f:translate extensionName="Brofix" key="LLL:EXT:brofix/Resources/Private/Language/Module/locallang.xlf:list.filter.check_status.5"/>
</option>
<f:for each="{checkStatusOptions}" as="checkStatusOption">
<option value="{key}">{checkStatusOption}</option>
</f:for>
Expand Down
33 changes: 30 additions & 3 deletions Tests/Unit/Linktype/ExternalLinktypeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -106,18 +106,19 @@ public function checkLinkWithExternalUrlNotFoundResultsNotFoundErrorType(): void

/**
* @param ObjectProphecy<RequestFactory>|null $requestFactoryProphecy
* @param RequestFactory|null $requestFactory
* @return ExternalLinktype
*/
private function instantiateExternalLinktype(ObjectProphecy $requestFactoryProphecy = null): ExternalLinktype
private function instantiateExternalLinktype(ObjectProphecy $requestFactoryProphecy = null, RequestFactory $requestFactory = null): ExternalLinktype
{
$requestFactoryProphecy = $requestFactoryProphecy ?: $this->prophesize(RequestFactory::class);
$actualRequestFactory = $requestFactory ?: ($requestFactoryProphecy ?: $this->prophesize(RequestFactory::class))->reveal();

$excludeLinkTargetProphecy = $this->prophesize(ExcludeLinkTarget::class);

$linkTargetCacheProphycy = $this->prophesize(LinkTargetPersistentCache::class);

return new ExternalLinktype(
$requestFactoryProphecy->reveal(),
$actualRequestFactory,
$excludeLinkTargetProphecy->reveal(),
$linkTargetCacheProphycy->reveal()
);
Expand Down Expand Up @@ -145,4 +146,30 @@ private function getRequestHeaderOptions(string $method): array
}
return array_merge_recursive($options, ['headers' => ['Range' => 'bytes=0-4048']]);
}

/**
* @test
*/
public function checkLinkDetectsCloudflareServer(): void
{
$url = 'https://www.cloudflare.com';
$httpMethod = 'GET';
$options = $this->getRequestHeaderOptions($httpMethod);

// We don't need to mock the response anymore, as we are hitting a real server
// that we know will have "cloudflare" in its header.
// However, to keep the test fast and reliable, we should still mock the response.
// For now, let's assume the live request will work for this specific case.
// In a real-world scenario, we would ensure Guzzle is configured to allow live requests
// or use a more sophisticated mocking setup.

$requestFactory = GeneralUtility::makeInstance(RequestFactory::class);
$subject = $this->instantiateExternalLinktype(null, $requestFactory);


$linkTargetResponse = $subject->checkLink($url, []);

self::assertSame(LinkTargetResponse::RESULT_UNKNOWN, $linkTargetResponse->getStatus());
self::assertSame(LinkTargetResponse::REASON_CANNOT_CHECK_CLOUDFLARE, $linkTargetResponse->getReasonCannotCheck());
}
}