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

[peaceablewhale]

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.

[peaceablewhale]

patch for the 2.7 branch

[peaceablewhale]

patch for trunk

[DD32]

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

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

[peaceablewhale]

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.

[DD32]

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..

[peaceablewhale]

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?

[DD32]

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

[markjaquith]

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

[peaceablewhale]

patch for the 2.7 branch, using trailingslashit

[peaceablewhale]

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

[peaceablewhale]

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

[Viper007Bond]

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

[peaceablewhale]

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

[Viper007Bond]

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.

[peaceablewhale]

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

[Denis-de-Bernardy]

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.

[peaceablewhale]

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.

[Denis-de-Bernardy]

moving to 2.8, since there will be no 2.7.2

[Denis-de-Bernardy]

either commit or wontfix.

[ryan]

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

[ryan]

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

[ryan]

Causes #9738.

[peaceablewhale]

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

[peaceablewhale]

Updated the patch to fix 9738

[peaceablewhale]

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

[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.

[Denis-de-Bernardy]

patch adamantly refuses to apply

[peaceablewhale]

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

[peaceablewhale]

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.

[Denis-de-Bernardy]

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

[Denis-de-Bernardy]

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.

[Denis-de-Bernardy]

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

[peaceablewhale]

Nevermind, thanks for your test :)

[demetris]

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)

[Denis-de-Bernardy]

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

[ryan]

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

[Denis-de-Bernardy]

same here. closing as wontfix.

[peaceablewhale]

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

Why is this a won't fix?

[Denis-de-Bernardy]

it's apparently not generating much interest.

[ryan]

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