Closed (fixed)
Project:
Drupal core
Version:
9.3.x-dev
Component:
link.module
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
18 May 2017 at 01:09 UTC
Updated:
18 Jan 2022 at 11:29 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
mac_weber commented.
Comment #3
mac_weber commentedComment #4
mac_weber commentedComment #6
mac_weber commentedPostponed in favor of the parent issue. The patch file was created over the last one in the parent issue and I would like to use the same test function.
Comment #13
digitaldonkey commentedUpdated Patch (only the front end Part was missing. Backend validation was comitted somehow else).
Comment #14
digitaldonkey commentedRe-add the new test from patch #3.
Sorry. This didn't work. #15 works, but does not have an additional test.
Comment #15
digitaldonkey commentedComment #16
tobiasbComment #18
stefan.kornSlightly changed patch taken from #3203656: Make uri required on the front-end when title filled-in. The patch from #15 does only apply if the link title is set to required, but it should also apply if the link title is set to optional. Link uri will still be required in that case.
Comment #19
ranjith_kumar_k_u commentedThe last patch works fine , when we type anything on the link text field the red asterisk will show above the URL field
Before patch

After patch

RTBC
Comment #20
stefan.kornanyone interested in this behavior without patching core can use Link Field Tweak module until this might get into core.
Comment #21
sonam.chaturvedi commentedComment #22
sonam.chaturvedi commentedVerified and tested patch#18. Patch applied successfully.
Testing Steps:
1. Goto "Create content" page with Link field
2. Check Link field is optional
3. Enter Link text field
4. Check URI is not required
5. Now apply patch#18
6. Again enter Link text field
7. Verify URI is required
Test Result: On entering value in Link text field > URI field is required
Before Patch:

After Patch:

Moving to RBTC
Comment #23
alexpottThanks for filing this bug report and for fixing the issue. Bug fixing is very valuable. In order to commit a bug fix, we need an automated test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal, see the following links:
I think several minor tweaks can be made to this code.
The if has lass brackets and work out if they mean anything. And less local variables.
Comment #25
meenakshi_j commentedAutomation testing needs to be required to implement this patch.
Comment #26
rinku jacob 13 commentedVerified and tested patch #25 for drupal 9.3.x-dev.Patch applied successfully thanks@Meenakshi_j. Adding screenshot for reference.
Comment #27
meenakshi_j commentedRequired Automation Testing.
Comment #28
vakulrai commentedThe LinkFiledTest has already a test written for the logic above, I think its good enough.
and After validating, the form is submitted with url only.
Comment #29
vikashsoni commentedApplied patch #25 working fine and applied successfully
Patch looks good for me
For ref sharing screenshots....
Comment #30
longwave@alexpott's comments in #23 have not yet been addressed.
Comment #31
tobiasbHere comes the test. ;-)
Comment #32
tobiasbFixed CS and change theme to stark.
Comment #33
longwaveThis patch fixes the frontend but is there any backend validation done here? The states API for required only shows/hides the asterisk, it doesn't actually do any validation when the form is submitted, as far as I'm aware.
This line is still confusing and @alexpott's changes haven't been done.
Comment #34
tobiasbThe server side is tested via
core/modules/link/tests/src/Functional/LinkFieldTest.php:315.So it is not possible to save the link just with the link text (without the patch).
And I do not see what is missing.
But I believe a better test class/method name would be better.
?
Comment #35
paulocsComment #36
paulocsI removed the unnecessary blank line and unnecessary brackets.
Btw @tobiasb is right about the server side validation. It has already.
I'm attaching a new patch, interdiff and a gif to confirm the server side validation (before patch is applied) which it has tests already.
Please review it.
Comment #37
longwaveThanks! The new test looks good to me and proves that the new #states configuration works as expected.
Comment #40
longwaveRandom fail, back to RTBC.
Comment #42
spokjeBack to RTBC per #40 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.
Comment #46
catchI removed some issue credit where screenshots were uploaded duplicating previous manual testing - this only needs to be done once (or not at all if there is sufficient automated test coverage).
Committed/pushed to 10.0.x and cherry-picked back through to 9.3.x, thanks!