WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 6 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 8 years ago.
patch for the 2.7 branch
link-template.php-trunk.patch (1.7 KB) - added by peaceablewhale 8 years ago.
patch for trunk
link-template.php-2.7-trailingslashit.patch (5.0 KB) - added by peaceablewhale 8 years ago.
patch for the 2.7 branch, using trailingslashit
link-template.php-trunk-trailingslashit.patch (5.1 KB) - added by peaceablewhale 8 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 8 years ago.
Updated the patch to fix 9738

Download all attachments as: .zip

Change History (44)

@peaceablewhale
8 years ago

patch for the 2.7 branch

@peaceablewhale
8 years ago

patch for trunk

#1 @peaceablewhale
8 years ago

  • Keywords has-patch added

#2 @DD32
8 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.

#3 @peaceablewhale
8 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.

#4 @DD32
8 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..

#5 @peaceablewhale
8 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?

#6 @DD32
8 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())

#7 @markjaquith
8 years ago

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

@peaceablewhale
8 years ago

patch for the 2.7 branch, using trailingslashit

@peaceablewhale
8 years ago

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

#8 @peaceablewhale
8 years ago

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

#9 @Viper007Bond
8 years ago

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

#10 follow-up: @peaceablewhale
8 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

#11 in reply to: ↑ 10 @Viper007Bond
8 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.

#12 follow-up: @peaceablewhale
8 years ago

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

#13 in reply to: ↑ 12 ; follow-up: @Denis-de-Bernardy
8 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.

#14 in reply to: ↑ 13 @peaceablewhale
8 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.

#15 @Denis-de-Bernardy
8 years ago

moving to 2.8, since there will be no 2.7.2

#16 @Denis-de-Bernardy
8 years ago

  • Milestone changed from 2.7.2 to 2.8

#17 @Denis-de-Bernardy
8 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.

#18 @ryan
8 years ago

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

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

#19 @ryan
8 years ago

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

#20 @ryan
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Causes #9738.

#21 @peaceablewhale
8 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?

@peaceablewhale
8 years ago

Updated the patch to fix 9738

#22 @peaceablewhale
8 years ago

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

#23 follow-up: @demetris
8 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.

#24 @Denis-de-Bernardy
8 years ago

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

patch adamantly refuses to apply

#25 @Denis-de-Bernardy
8 years ago

  • Milestone changed from 2.8 to Future Release

#26 @peaceablewhale
8 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!

#27 @peaceablewhale
8 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.

#28 @Denis-de-Bernardy
8 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

#29 @Denis-de-Bernardy
8 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.

#30 @Denis-de-Bernardy
8 years ago

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

#31 @peaceablewhale
8 years ago

Nevermind, thanks for your test :)

#32 in reply to: ↑ 23 @demetris
8 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)

#33 @Denis-de-Bernardy
8 years ago

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

#34 @ryan
8 years ago

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

#35 @Denis-de-Bernardy
8 years ago

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

same here. closing as wontfix.

#36 @peaceablewhale
8 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?

#37 @Denis-de-Bernardy
8 years ago

  • Milestone set to 2.8

it's apparently not generating much interest.

#38 @ryan
8 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

#39 @peaceablewhale
6 years ago

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