Follow up for #1916892: Capitalize node admin "edit" link into "Edit"

Problem/Motivation

clickLink() api says it's case insensitive. but it's not.

Proposed resolution

update docs

Remaining tasks

patch done.
review next.

User interface changes

No ui changes.

API changes

No api changes.

Files: 
CommentFileSizeAuthor
#31 document_that_clickLink___is_case_sensitive-1916928-31.patch896 bytestoastieAlex
PASSED: [[SimpleTest]]: [MySQL] 39,931 pass(es).
[ View ]
#18 1916928_15.patch3.78 KBchx
PASSED: [[SimpleTest]]: [MySQL] 50,639 pass(es).
[ View ]
#13 interdiff.txt1.98 KBdawehner
#13 drupal-1916928-13.patch2.92 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 50,758 pass(es).
[ View ]
#10 1916928_10.patch957 byteschx
FAILED: [[SimpleTest]]: [MySQL] 50,393 pass(es), 14 fail(s), and 0 exception(s).
[ View ]
#6 drupal-1916928-6.patch945 bytesdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,129 pass(es), 209 fail(s), and 60 exception(s).
[ View ]
#3 1916928_3.patch863 byteschx
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php.
[ View ]
#1 clickLink_docs_incorrect_it_s_case_sensitive-1916928-1.patch999 bytesYesCT
PASSED: [[SimpleTest]]: [MySQL] 50,413 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new999 bytes
PASSED: [[SimpleTest]]: [MySQL] 50,413 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

This is correct. XPath 1.0 is always case sensitive.

Title:clickLink() docs incorrect, it's case sensitiveclickLink() is case sensitive despite it should be insensitive
Component:documentation» simpletest.module
Status:Reviewed & tested by the community» Needs review
StatusFileSize
new863 bytes
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php.
[ View ]

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -1920,7 +1920,7 @@ protected function assertNoLinkByHref($href, $message = '', $group = 'Other') {
+    $urls = $this->xpath('//a[normalize-space(translate(text(), 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz'))=:label]', array(':label' => $label));

What about document why we are doing that here?

Status:Needs review» Needs work

The last submitted patch, 1916928_3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new945 bytes
FAILED: [[SimpleTest]]: [MySQL] 50,129 pass(es), 209 fail(s), and 60 exception(s).
[ View ]

Fixed the php code and added a comment.

is transliteration going to complicate this? Links could be using other characters?

Status:Needs review» Needs work

The last submitted patch, drupal-1916928-6.patch, failed testing.

My guess is those fails are all the tests that need to have their offset's adjusted now that it is case insensitive.

Status:Needs work» Needs review
StatusFileSize
new957 bytes
FAILED: [[SimpleTest]]: [MySQL] 50,393 pass(es), 14 fail(s), and 0 exception(s).
[ View ]

If the left is converted to lower the right should be as well. strtolower() added.

Status:Needs review» Needs work

The last submitted patch, 1916928_10.patch, failed testing.

My guess is those fails are all the tests that need to have their offset's adjusted now that it is case insensitive. :P

Status:Needs work» Needs review
StatusFileSize
new2.92 KB
PASSED: [[SimpleTest]]: [MySQL] 50,758 pass(es).
[ View ]
new1.98 KB

This should fix all of them.

Status:Needs review» Reviewed & tested by the community

ok. this looks good. offsets are added, passes testbot, and the couple lines of code change seem ok standards-wise.

Won't the transliteration break for non-english?

@twistor I wondered similarly in #7

Seems this is as close to case insensitive as we can get without specifying a language.

Good Enough for now I think.

@YesCT Oh, I guess you did. I'm just wondering about the trade-off. Fixing docs vs. running tests on non-English sites.

The new code is correct, if that's the direction we want to go, so still RTBC.

StatusFileSize
new3.78 KB
PASSED: [[SimpleTest]]: [MySQL] 50,639 pass(es).
[ View ]

Rewrote the doxygen on clickLink.

nice. (still rtbc)

@15,@16
Is this actually a real problem, if you assume that actually the strings should be english in those tests? If they aren't english you will have the problem already during running tests first so you can adapt the $index as you need.

@dawehner yep. the other choice is probably to leave it with the patch from #1.
What do you think though?

Status:Reviewed & tested by the community» Needs review

Why does it document that it is case-insensitive in the first place?

This is a test assertion method. Tests are for asserting expectations. Therefore, clickLink() should be case-sensitive.

Let's fix the phpDoc instead.

Status:Needs review» Reviewed & tested by the community

So we have a patch in #1 for that.

I have a slight preference for #1 since it behaves the same no matter the language. But either approach is an improvement.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed #1 to 8.x. Moving to 7.x.

Title:clickLink() is case sensitive despite it should be insensitiveDocument clickLink() is case sensitive
Component:simpletest.module» documentation
Category:bug» task
Status:Patch (to be ported)» Active

Dries, this is not backportable, #10 shows that tests break when this change happens. So, I wrote a change notice and set this to documentation for 7.x.

And this is a novice task for sure: you need to change "insensitive" to "sensitive" but also I would love if the other doxygen changes in #18 would also be enrolled.

Title:Document clickLink() is case sensitiveDocument that clickLink() is case sensitive

@chx
The behavior did not changed at all, see http://drupal.org/node/1916928#comment-7074496

Oh. OK, change notice nuked :) I completely missed that #1 was committed.

Issue tags:+Novice

Cool.
Adding a novice tag for the backport.

Status:Active» Needs review
StatusFileSize
new896 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,931 pass(es).
[ View ]

7.x backport patch. Good place for this novice to start contributing.

Status:Needs review» Reviewed & tested by the community

@Alexander Pyle Thanks! And welcome!

code looks good: changes the text to specify case sensitive (and re-wraps to 80 chars).
also testbot says green. :)
So, RTBC.

Assigned:Unassigned» jhodgdon

Thanks! I'll get this committed shortly.

Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Fixed

Backport committed to 7.x. Thanks again!

Automatically closed -- issue fixed for 2 weeks with no activity.