Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#31 | document_that_clickLink___is_case_sensitive-1916928-31.patch | 896 bytes | toastieAlex |
#18 | 1916928_15.patch | 3.78 KB | chx |
#13 | interdiff.txt | 1.98 KB | dawehner |
#13 | drupal-1916928-13.patch | 2.92 KB | dawehner |
#10 | 1916928_10.patch | 957 bytes | chx |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedComment #2
twistor CreditAttribution: twistor commentedThis is correct. XPath 1.0 is always case sensitive.
Comment #3
chx CreditAttribution: chx commentedNo Xpath 2.0 in PHP libxml.
Comment #4
dawehnerWhat about document why we are doing that here?
Comment #6
dawehnerFixed the php code and added a comment.
Comment #7
YesCT CreditAttribution: YesCT commentedis transliteration going to complicate this? Links could be using other characters?
Comment #9
YesCT CreditAttribution: YesCT commentedMy guess is those fails are all the tests that need to have their offset's adjusted now that it is case insensitive.
Comment #10
chx CreditAttribution: chx commentedIf the left is converted to lower the right should be as well. strtolower() added.
Comment #12
chx CreditAttribution: chx commentedMy guess is those fails are all the tests that need to have their offset's adjusted now that it is case insensitive. :P
Comment #13
dawehnerThis should fix all of them.
Comment #14
YesCT CreditAttribution: YesCT commentedok. this looks good. offsets are added, passes testbot, and the couple lines of code change seem ok standards-wise.
Comment #15
twistor CreditAttribution: twistor commentedWon't the transliteration break for non-english?
Comment #16
YesCT CreditAttribution: YesCT commented@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.
Comment #17
twistor CreditAttribution: twistor commented@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.
Comment #18
chx CreditAttribution: chx commentedRewrote the doxygen on clickLink.
Comment #19
YesCT CreditAttribution: YesCT commentednice. (still rtbc)
Comment #20
dawehner@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.
Comment #21
YesCT CreditAttribution: YesCT commented@dawehner yep. the other choice is probably to leave it with the patch from #1.
What do you think though?
Comment #22
sunWhy 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.
Comment #23
dawehnerSo we have a patch in #1 for that.
Comment #24
YesCT CreditAttribution: YesCT commentedI have a slight preference for #1 since it behaves the same no matter the language. But either approach is an improvement.
Comment #25
Dries CreditAttribution: Dries commentedCommitted #1 to 8.x. Moving to 7.x.
Comment #26
chx CreditAttribution: chx commentedDries, 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.
Comment #27
chx CreditAttribution: chx commentedAnd 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.
Comment #28
dawehner@chx
The behavior did not changed at all, see http://drupal.org/node/1916928#comment-7074496
Comment #29
chx CreditAttribution: chx commentedOh. OK, change notice nuked :) I completely missed that #1 was committed.
Comment #30
dawehnerCool.
Adding a novice tag for the backport.
Comment #31
toastieAlex CreditAttribution: toastieAlex commented7.x backport patch. Good place for this novice to start contributing.
Comment #32
YesCT CreditAttribution: YesCT commented@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.
Comment #33
jhodgdonThanks! I'll get this committed shortly.
Comment #34
jhodgdonBackport committed to 7.x. Thanks again!