Paths

Table of Contentst

Differential D25139

Fix is_absolute test in markup
ClosedPublic
Actions

Authored by avivey on Apr 28 2023, 10:44.

Tags

None

Referenced Files

	Unknown Object (File)
	Sun, Apr 7, 21:38

	Unknown Object (File)
	Sat, Apr 6, 22:37

	Unknown Object (File)
	Mon, Apr 1, 05:04

	Unknown Object (File)
	Mon, Apr 1, 02:57

	Unknown Object (File)
	Mon, Apr 1, 01:45

	Unknown Object (File)
	Sun, Mar 31, 07:04

	Unknown Object (File)
	Sun, Mar 31, 03:09

	Unknown Object (File)
	Wed, Mar 27, 18:04

View All 84 Files

Subscribers

valerio.bozzolan

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Commits

rP74dfe6f971af: Fix is_absolute test in markup

Summary

See Q53. Fixes rP935d7120ee32.

Call sites should be happy to use PhutilURI when possible.

Test Plan

visit any Repository page. Before - exception, now - data.

Diff Detail

Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

avivey created this revision.Apr 28 2023, 10:44

Owners added a reviewer: O1: Blessed Committers.Apr 28 2023, 10:44

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptApr 28 2023, 10:44

avivey requested review of this revision.Apr 28 2023, 10:44

Harbormaster completed remote builds in B276: Diff 538.Apr 28 2023, 10:44

avivey edited the summary of this revision. (Show Details)Apr 28 2023, 10:45

lint

Harbormaster completed remote builds in B277: Diff 539.Apr 28 2023, 10:46

Thank you, it seems also a bit more efficient since there is no need to run a preg match for that case.

With that strict check I will +1 as O1

src/infrastructure/javelin/markup.php
81	I would recommend a strict check here to avoid type juggling (0 == null) is true etc. https://www.php.net/manual/en/types.comparisons.php

Even if probably it would be nice to have an isAbsoluteURI() method in PhutilURI to reduce the business logic here

In D25139#4421, @valerio.bozzolan wrote:

Even if probably it would be nice to have an isAbsoluteURI() method in PhutilURI to reduce the business logic here

I looked into it, but (1) it's in a different repo, so the fix would be harder and (2) the test here isn't really "is absolute" but rather "does this look like a web URI". PhutilURI is also used for git/svn/ssh URIs, which would be "absolute" and also make no sense here.

src/infrastructure/javelin/markup.php
81	👍

This revision was not accepted when it landed; it landed in state Needs Review.Apr 28 2023, 11:10

Closed by commit rP74dfe6f971af: Fix is_absolute test in markup. · Explain Why

This revision was automatically updated to reflect the committed changes.

avivey added a commit: rP74dfe6f971af: Fix is_absolute test in markup.

I would like to approve this but it's too late :D thanks

valerio.bozzolan mentioned this in Next Up (since Week 18).Apr 28 2023, 11:16

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 28 2023, 20:13

Revision Contents
Changeset List

Path

Size

Packages

src/

infrastructure/

javelin/

22 lines

O1: Blessed Committers

Diff 540

src/infrastructure/javelin/markup.php

Loading...