Paths

Table of Contentst

Differential D11338

Add default ports to `PhutilURI`
ClosedPublic
Actions

Authored by joshuaspence on Jan 12 2015, 2:37 AM.

Tags

None

Referenced Files

	Unknown Object (File)
	Sat, Nov 23, 3:57 PM

	Unknown Object (File)
	Mon, Oct 28, 10:04 AM

	Unknown Object (File)
	Oct 20 2024, 7:50 PM

	Unknown Object (File)
	Oct 16 2024, 8:16 PM

	Unknown Object (File)
	Oct 15 2024, 9:11 AM

	Unknown Object (File)
	Oct 15 2024, 9:08 AM

	Unknown Object (File)
	Oct 15 2024, 9:07 AM

	Unknown Object (File)
	Oct 15 2024, 9:05 AM

Subscribers

Details

Reviewers

Group Reviewers

Blessed Reviewers

Commits

rPHUd7e7df98d2ea: Add default ports to `PhutilURI`

Summary

Add default ports to PhutilURI to allow the getPort() method to return the default port is no port number is explicitly provided.

Test Plan

Added test cases.

Diff Detail

Repository

Branch

master

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 3775
Build 3786: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence updated this revision to Diff 27239.Jan 12 2015, 2:37 AM

joshuaspence retitled this revision from to Add default ports to `PhutilURI`.

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJan 12 2015, 2:37 AM

Herald added subscribers: epriestley, Korvin. · View Herald Transcript

Minor change

I think this should be on a separate call, e.g. getPortWithProtocolDefault() or something less wordy.

As implemented, this discards information (whether the port was explicit or not). We have some cases (offhand, OAuth2 redirect URI verification and Conduit request signing) where there may be a difference between "no port specified" and "port 80, explicitly" in a URI.

This revision now requires changes to proceed.Jan 12 2015, 4:01 PM

What about function getPort($lookup_default = false)?

I like it as a separate method, I'd never guess what getPort(true) meant without knowing.

Move to a new method

epriestley accepted this revision.Jan 12 2015, 7:52 PM

epriestley edited edge metadata.

This revision is now accepted and ready to land.Jan 12 2015, 7:52 PM

Yeah, I couldn't come up with a better name for this method.

Closed by commit rPHUd7e7df98d2ea: Add default ports to `PhutilURI` (authored by joshuaspence, committed by joshuaspence). · Explain WhyJan 12 2015, 7:54 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

src/

parser/

10 lines

__tests__/

PhutilURITestCase.php

12 lines

Diff 27242

src/parser/PhutilURI.php

Loading...

src/parser/tests/PhutilURITestCase.php

Loading...