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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Status: Active » Needs review
FileSize
999 bytes
twistor’s picture

Status: Needs review » Reviewed & tested by the community

This is correct. XPath 1.0 is always case sensitive.

chx’s picture

Title: clickLink() docs incorrect, it's case sensitive » clickLink() is case sensitive despite it should be insensitive
Component: documentation » simpletest.module
Status: Reviewed & tested by the community » Needs review
FileSize
863 bytes
dawehner’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
945 bytes

Fixed the php code and added a comment.

YesCT’s picture

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.

YesCT’s picture

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

chx’s picture

Status: Needs work » Needs review
FileSize
957 bytes

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.

chx’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
1.98 KB

This should fix all of them.

YesCT’s picture

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.

twistor’s picture

Won't the transliteration break for non-english?

YesCT’s picture

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

twistor’s picture

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

chx’s picture

FileSize
3.78 KB

Rewrote the doxygen on clickLink.

YesCT’s picture

nice. (still rtbc)

dawehner’s picture

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

YesCT’s picture

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

sun’s picture

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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

So we have a patch in #1 for that.

YesCT’s picture

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

Dries’s picture

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.

chx’s picture

Title: clickLink() is case sensitive despite it should be insensitive » Document 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.

chx’s picture

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.

dawehner’s picture

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

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

chx’s picture

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

dawehner’s picture

Issue tags: +Novice

Cool.
Adding a novice tag for the backport.

toastieAlex’s picture

Status: Active » Needs review
FileSize
896 bytes

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

YesCT’s picture

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.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Thanks! I'll get this committed shortly.

jhodgdon’s picture

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.