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.We broke the output of pseudofield flags and didn't notice (#2599304: [regression] Reimplement Contextual Link Display). We're missing tests for the various options that allow a flag to show in different places:
- entity links, each view mode
- pseudofield, enabled and disabled
- entity form (probably not working right now)
- contextual links (patch here: #2599304: [regression] Reimplement Contextual Link Display)
I think the best thing here is a new test class that covers just this area -- called something like OutputLocationTest maybe.
Test should create a flag with the reload link type (as that's the simplest). We don't need to test the link works -- that's the job of other tests. We just need to check the link outputs as expected. We also need to check the link *doesn't* output, so there should be one step where all the options are disabled and the flag link text should not show.
So:
- create flag programmatically, set it to not show anywhere
- create node that's flaggable
- check teaser and full views modes for the flag link: it should not show
- set the flag link to show in entity links, one mode at a time, checking each time
- set the flag link to show as a pseudofield. Disable it, then enable it in field view options.
Also, there's a bit of testing for this in FlagSimpleTest::doFlagLinkTeaserTest(), which should be removed from there and incorporated into the new tests.
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | interdiff-2600146-14-18.txt | 1.61 KB | martin107 |
| #18 | 2600146-18.flag_.link-location-tests.patch | 8.41 KB | martin107 |
| |||
| #14 | interdiff-2600146-12-14.txt | 5.1 KB | martin107 |
| #14 | 2600146-14.flag_.link-location-tests.patch | 8.41 KB | martin107 |
| |||
| #12 | 2600146-12.flag_.link-location-tests.patch | 8.01 KB | joachim |
| |||
Comments
Comment #2
joachim CreditAttribution: joachim commentedI am totally stumped on this at the moment.
I am using the test xpath() method to check the flag link, as we need to differentiate between the link as a pseudofield, and the link within the entity links.
When I open the page as seen by the test browser, this xpath expression works (with a Firefox xpath plugin):
"//*[contains(@class, 'node__content')]/a[@id = ':id']"but it fails in the test!
I'm posting my work so far in the hope someone can help me figure this out :)
Comment #3
nlisgo CreditAttribution: nlisgo commentedComment #4
nlisgo CreditAttribution: nlisgo commentedJust needed to remove the single quotes around the placeholder :id.
Comment #5
joachim CreditAttribution: joachim commentedTHANK YOU!!!! :)
Comment #6
nlisgo CreditAttribution: nlisgo commentedYou're very welcome. Happy to help.
Comment #7
emclaughlin CreditAttribution: emclaughlin at Genuine commentedI think some of the TODOs should be filled out before this can actually be committed? But I mean, beyond TODOs, it looks good to me so if y'all are okay with those being a separate issue, then this is RBTC by me. But I'm setting it back to Needs Work until someone with more authority than me weighs in.
Comment #8
joachim CreditAttribution: joachim commentedNo, this is still a work in progress. I just got stuck on the xpath bit.
Comment #9
joachim CreditAttribution: joachim commentedFinished -- as much as can be until #2411977: [regression] Reimplement Entity links Display is fixed.
Comment #10
joachim CreditAttribution: joachim commentedTests pass, but it would be good to get a review of this, just to check that it tests the things it ought to be testing.
Comment #11
joachim CreditAttribution: joachim commentedFlagSimpleTest::doFlagLinkTeaserTest() should be removed, as that's covered in the new class this patch adds.
Comment #12
joachim CreditAttribution: joachim commentedDone.
Comment #13
joachim CreditAttribution: joachim commentedComment #14
martin107 CreditAttribution: martin107 commentedFrom #10
The separation of test concerns makes sense to me.... and we get coverage that is appropriate.. yes there are lots of todos but that is ok it is in a good direction
so plus one from me...
My changes are just minor nit picks
1) I have locked down the input parameter requirements for various LinkOutputLocationTest::assertXYZ() methods and spruced up the method annotations.
2) I have changed a variable name ... LinkOutputLocationTest::testLinkLocation()::$flag_config. As it was showing up in my lint tool as an inappropriate reuse of a unconnected local variable. This is a trivial maintenance hazard ... if a config option needs to be added to the configuration at the top of the method then something unwanted could bleed into the second configuration.
Naming things is hard so if you object to the variable names that is fine ... please let me know and I will jump on it.
3) LinkOutputLocationTest::$node - I have changed the propery type to NodeInterface ... I just want to imply flexibility, while the call to Node::create() will generate an instance of \Drupal\node\Entity\Node I want to suggest that our test will work on anything conforming to the interface.
4) Make little changes seen while running LinkOutputLocationTest through phpcs.
Comment #15
socketwench CreditAttribution: socketwench as a volunteer commentedI like this. Again, there's a lot of TODOs, but it's clear what is intended to go where as we solve issues.
Comment #16
joachim CreditAttribution: joachim commentedThanks for all those cleanups, @martin107!
There's just one I'm not keen on:
> 2) I have changed a variable name ... LinkOutputLocationTest::testLinkLocation()::$flag_config. As it was showing up in my lint tool as an inappropriate reuse of a unconnected local variable. This is a trivial maintenance hazard ... if a config option needs to be added to the configuration at the top of the method then something unwanted could bleed into the second configuration.
I see this as the same way that each time you do a drupalPost, you make an $edit array. Having the variable name change each time means that it's harder to copy-paste bits of the code to further tests in future, and I think it makes it harder to follow the repetitive pattern of the test.
I'll reroll :)
Comment #17
martin107 CreditAttribution: martin107 commentedIf you don;t mind waiting until tonight ... I will revert
( I am quite happy to backout things that fail review.)
Comment #18
martin107 CreditAttribution: martin107 commentedReverted #14.2
Comment #19
joachim CreditAttribution: joachim commentedThanks for the reroll :) And thanks @socketwench for the review too.
Committed! -- with just a small tweak for a missing blank line after the import statements.