WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#9515 closed defect (bug) (fixed)

feed_link functions should add a slash before the query when permalink structure is default

Reported by: peaceablewhale Owned by: ryan
Milestone: 2.8 Priority: low
Severity: minor Version: 2.7.1
Component: Permalinks Keywords:
Focuses: Cc:

Description

Currently, get_author_feed_link, get_category_feed_link, get_tag_feed_link, get_search_feed_link and get_search_comments_feed_link generate links link http://blog.example.org?s=test&feed=atom.

The canonical form of the link above is http://blog.example.org/?s=test&feed=atom.

Therefore, feed_link functions should add a slash before the query when permalink structure is default.

Attachments (5)

link-template.php-2.7.patch (1.6 KB) - added by peaceablewhale 6 years ago.
patch for the 2.7 branch
link-template.php-trunk.patch (1.7 KB) - added by peaceablewhale 6 years ago.
patch for trunk
link-template.php-2.7-trailingslashit.patch (5.0 KB) - added by peaceablewhale 6 years ago.
patch for the 2.7 branch, using trailingslashit
link-template.php-trunk-trailingslashit.patch (5.1 KB) - added by peaceablewhale 6 years ago.
patch for the trunk, using trailingslashit, updated at 2009-04-12 05:06 UTC
link-template.php-trunk-user_trailingslashit.patch (5.1 KB) - added by peaceablewhale 6 years ago.
Updated the patch to fix 9738

Download all attachments as: .zip

Change History (44)

@peaceablewhale6 years ago

patch for the 2.7 branch

@peaceablewhale6 years ago

patch for trunk

comment:1 @peaceablewhale6 years ago

  • Keywords has-patch added

comment:2 @DD326 years ago

get_option('home') might already have a trailing slash.

trailingslashit( get_option('home') ) would be the correct way of doing it in that case.

comment:3 @peaceablewhale6 years ago

However, according to the documents, get_option('home') must not have a trailing slash, no matter whether it is input through the admin panel or the WP_HOME constant in wp-config.php.

comment:4 @DD326 years ago

hm. I think the get_option() actually has an untrailingslashit() on it anyway to enforce that too..I forgot about that.

Eitherway, I still think its best to do it that way, It adds a few tenths of a millisecond to the processing, but its a cleaner way of going about it IMO..

comment:5 @peaceablewhale6 years ago

Many other functions (such as _get_page_link, get_attachment_link, get_feed_link, etc.) in link-template.php also use get_option('home')."/". Should they be changed as well?

comment:6 @DD326 years ago

  • Keywords dev-feedback added
  • Milestone changed from Unassigned to 2.7.2

Can a core-dev comment on which way to go? (Hardcoded / or trailinslashit())

comment:7 @markjaquith6 years ago

Go with trailingslashit(), as it will prevent edge cases of double //.

@peaceablewhale6 years ago

patch for the 2.7 branch, using trailingslashit

@peaceablewhale6 years ago

patch for the trunk, using trailingslashit, updated at 2009-04-12 05:06 UTC

comment:8 @peaceablewhale6 years ago

link-template.php-trunk-trailingslashit.patch was updated at 2009-04-12 05:06 UTC to add a correction for line 228.

comment:9 @Viper007Bond6 years ago

Shouldn't user_trailingslashit() be used here? Not all users want a trailing slash.

comment:10 follow-up: @peaceablewhale6 years ago

I think the trailing slash is always required:

get_option('home'): http://blog.example.org
get_tag_feed_link(1,'atom')--
Current behavior: http://blog.example.org?feed=atom&tag=1
Expected behavior: http://blog.example.org/?feed=atom&tag=1

get_option('home'): http://www.example.org/blog
get_tag_feed_link(1,'atom')--
Current behavior: http://www.example.org/blog?feed=atom&tag=1
Expected behavior: http://www.example.org/blog/?feed=atom&tag=1

comment:11 in reply to: ↑ 10 @Viper007Bond6 years ago

Replying to peaceablewhale:

I think the trailing slash is always required:

No it's not. Infact WordPress enforces and will remove the trailing slash from post URLs if you opt not to have one in your structure at Settings -> Permalinks. That is the whole purpose of user_trailingslashit() (it'll only add one if your permalink structure has one).

However this may not be the case for the root of your blog. I'm not sure if the redirect from http://example.com/blog to http://example.com/blog/ is being done by WordPress or the HTTPD (I assume the latter).

get_tag_feed_link() should definitely include a trailing slash if your permalink structure has one, but I'm not sure if it should or not if your structure doesn't.

comment:12 follow-up: @peaceablewhale6 years ago

My modifications apply to $permalink_structure=="" only, does it mean that the permalink structure always has a trailing slash?

comment:13 in reply to: ↑ 12 ; follow-up: @Denis-de-Bernardy6 years ago

Replying to peaceablewhale:

My modifications apply to $permalink_structure=="" only, does it mean that the permalink structure always has a trailing slash?

not necessarily. %postname%.htm will output page links without a trailing slash.

comment:14 in reply to: ↑ 13 @peaceablewhale6 years ago

Replying to Denis-de-Bernardy:

not necessarily. %postname%.htm will output page links without a trailing slash.

Thanks Denis. I have uploaded a new patch using user_trailingslashit.

comment:15 @Denis-de-Bernardy6 years ago

moving to 2.8, since there will be no 2.7.2

comment:16 @Denis-de-Bernardy6 years ago

  • Milestone changed from 2.7.2 to 2.8

comment:17 @Denis-de-Bernardy6 years ago

  • Keywords tested commit added; dev-feedback removed
  • Priority changed from normal to low
  • Severity changed from normal to minor

either commit or wontfix.

comment:18 @ryan6 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [11210]) Slashe before query args. Props peaceablewhale. fixes #9515

comment:19 @ryan6 years ago

(In [11218]) Revert[11210]. fixes #9738 see #9515

comment:20 @ryan6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Causes #9738.

comment:21 @peaceablewhale6 years ago

  • Keywords needs-testing dev-feedback added; tested commit removed

I think link-template.php-trunk-trailingslashit.patch is a better fix in that case. Any comment?

@peaceablewhale6 years ago

Updated the patch to fix 9738

comment:22 @peaceablewhale6 years ago

I have modified the patch to use trailingslashit instead of user_trailingslashit in line 228, which should fix #9738 properly.

comment:23 follow-up: @demetris6 years ago

  • Keywords tested added; needs-testing removed

New patch to fix 9738 seems OK.

If there are any other side-effects, I think we’ll be able to catch them until 2.8 final.

comment:24 @Denis-de-Bernardy6 years ago

  • Keywords needs-patch added; has-patch tested removed

patch adamantly refuses to apply

comment:25 @Denis-de-Bernardy6 years ago

  • Milestone changed from 2.8 to Future Release

comment:26 @peaceablewhale6 years ago

  • Milestone changed from Future Release to 2.8

Hey Denis, please provide additional information on how the patch fails, and please don't change the milestone. Thanks!

comment:27 @peaceablewhale6 years ago

  • Keywords has-patch added; needs-patch removed

I have checked that the patch applies successfully and therefore I have restored the "has-patch" keyword. The "tested" keyword will be restored 24 hours later if there is no additional information.

comment:28 @Denis-de-Bernardy6 years ago

no idea what it rejects it, but:

DB:~/Sites/wp $ svn up

Fetching external item into 'wp-content/plugins/akismet'
External at revision 117154.

At revision 11262.

DB:~/Sites/wp $ wptest http://core.trac.wordpress.org/attachment/ticket/9515/link-template.php-trunk-user_trailingslashit.patch
(Stripping trailing CRs from patch.)
patching file link-template.php
Hunk #1 FAILED at 154.
Hunk #2 FAILED at 225.
Hunk #3 FAILED at 269.
Hunk #4 FAILED at 292.
Hunk #5 FAILED at 317.
Hunk #6 FAILED at 347.
Hunk #7 FAILED at 382.
Hunk #8 FAILED at 414.
Hunk #9 FAILED at 468.
Hunk #10 FAILED at 512.
Hunk #11 FAILED at 551.
Hunk #12 FAILED at 626.
Hunk #13 FAILED at 651.
13 out of 13 hunks FAILED -- saving rejects to file link-template.php.rej

DB:~/Sites/wp $ cat ~/bin/wptest 
#!/bin/sh

wpreset && wppatch $*

DB:~/Sites/wp $ cat ~/bin/wpreset 
#!/bin/sh

find . -name *.diff -exec rm '{}' \;
find . -name *.patch -exec rm '{}' \;
find . -name *.rej -exec rm '{}' \;
find . -name *.orig -exec rm '{}' \;
svn revert -R * -q

DB:~/Sites/wp $ cat ~/bin/wppatch
#!/bin/sh
#
# Automatically download and apply a patch from core.trac.wordpress.org
#
# Usage:
#
#   wp_patch http://core.trac.wordpress.org/attachment/ticket/132/132.diff
#   wp_patch 123			-- http://core.trac.wordpress.org/attachment/ticket/132/132.diff
#   wp_patch 123.2.diff		-- http://core.trac.wordpress.org/attachment/ticket/132/132.2.diff
#   wp_patch 123 456.diff	-- http://core.trac.wordpress.org/attachment/ticket/132/456.diff
#

if [ ! -f wp-config.php ];
then
	echo 'Invalid WP install'
	exit 1
fi

case $#
in
	1 )
		ticket=$1
		if [ $ticket != ${ticket%/*} ];
		then
			ticket=${ticket##*/ticket/}
			patch=${ticket##*/}
			ticket=${ticket%%/*}
		elif [ $ticket != ${ticket%%.*} ];
		then
			patch=$ticket
			ticket=${ticket%%.*}
		else
			patch=$ticket.diff
		fi
		;;
	2 )
		ticket=$1
		patch=$2
		;;
	* )
		echo 'Invalid number of args'
		exit 1
esac

curl -s --connect-timeout 3 http://core.trac.wordpress.org/raw-attachment/ticket/$ticket/$patch -o $PWD/$patch

if [ $? -ne 0 ];
then
	echo 'Error downloading patch'
	exit 1
fi

patch -p0 -i $PWD/$patch && rm $PWD/$patch

comment:29 @Denis-de-Bernardy6 years ago

  • Keywords tested added; dev-feedback removed

my bad, and sorry about that. upon downloading it and applying into the wp-includes folder, it applies cleanly. gotta look into why patch isn't prompting to change the file's name as it normally does.

comment:30 @Denis-de-Bernardy6 years ago

k, and now I know. there was a link-template.php file in my wp trunk for some reason. :-)

comment:31 @peaceablewhale6 years ago

Nevermind, thanks for your test :)

comment:32 in reply to: ↑ 23 @demetris6 years ago

  • Cc dkikizas@… added
  • Keywords needs-patch added; has-patch tested removed

Replying to demetris:

New patch to fix 9738 seems OK.

If there are any other side-effects, I think we’ll be able to catch them until 2.8 final.

I’ve been using this patch since my last report and today I noticed another issue:

IF a site is installed in a subdirectory AND the default permalink structure is used, THEN the generated permalinks miss one slash. They are like this:

http://example.net/subdir?p=1

(WP redirects them properly to their canonical locations.)

Compare these two test sites, both of which have the patch applied and use the default permalink structure:

http://op111.net/x/ — (Has the problem)

http://op109.net/ — (Does not have the problem)

comment:33 @Denis-de-Bernardy6 years ago

k... itching to close this one as dup of #9825

comment:34 @ryan6 years ago

Personally, I'd prefer to always use trailingslashit(). example.com?s=blah looks goofy to me.

comment:35 @Denis-de-Bernardy6 years ago

  • Milestone 2.8 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

same here. closing as wontfix.

comment:36 @peaceablewhale6 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

There is actually a patch that always use trailingslashit(): link-template.php-trunk-trailingslashit.patch

Why is this a won't fix?

comment:37 @Denis-de-Bernardy6 years ago

  • Milestone set to 2.8

it's apparently not generating much interest.

comment:38 @ryan6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [11364]) Add trailing slash before query args for crufty links. Props peaceablewhale. fixes #9515

comment:39 @peaceablewhale5 years ago

  • Keywords needs-patch removed
Note: See TracTickets for help on using tickets.