Get unique html element id from record view helper #2999
Conversation
demiankatz
changed the title
There was a problem hiding this comment.
Thanks for getting this started, @RLangeUni -- I think this has turned up some issues that require a little more work. See comments below!
| @@ -415,8 +415,7 @@ public function getSearchResult($view) | |||
| */ | |||
| public function getCheckbox($idPrefix = '', $formAttr = false, $number = null) | |||
| { | |||
| $id = $this->driver->getSourceIdentifier() . '|' | |||
| . $this->driver->getUniqueId(); | |||
| $id = $this->getUniqueHtmlElementId($idPrefix); | |||
There was a problem hiding this comment.
I don't think this is working correctly, either in the existing dev code or in this PR. The $id value is the record identifier intended to be used as the VALUE of the checkbox. The $idPrefix value is a prefix for the HTML ID of the checkbox, but currently, the record/checkbox.phtml template does not set any HTML IDs at all or make use of the 'prefix' value in the context! We definitely do NOT want to prefix the $id value, since that is a reference to a specific record in VuFind, and prefixing it will break functionality related to it. We probably DO want to add an HTML element ID, but we'll need to adjust the template to accept it in that case. I've gone all the way back through history, and this has NEVER been set up correctly. The $idPrefix variable has been present in the helper and unused in the template since the bootstrap3/bootprint3 themes were introduced in 2014. Maybe the solution is something like this:
$context = compact('number') + [
'id' => $this->getUniqueIdWithSourcePrefix(),
'checkboxElementId' => $this->getUniqueHtmlElementId($idPrefix),
];
|
@elsenhans and @RLangeUni, there hasn't been further activity here for more than a month -- just wanted to be sure you saw my earlier review. Please let me know if you need any more action/information from my end! |
|
Hi, @demiankatz , thanks for the reminder! |
|
Thanks, @elsenhans -- sorry that #2982 took so long to resolve, but now that it's finally done, hopefully we can make more progress here. |
|
I'm moving this to the 10.0 milestone, as we're getting close to the 9.1 code freeze, and I don't think this is going to get done in time. Of course, happy to keep working on it regardless whenever you're ready! |
|
@RLangeUni / @elsenhans, now that 10.0 development is beginning, I'm just checking in to see where we are on this PR. Please let me know if you need any action on my end to get this moving again. |
|
@RLangeUni , @demiankatz |
|
@elsenhans, see my review comments above for the specific issues I highlighted on my previous review. As you say, all of this is touching PHP code, but it is PHP code that is used to set values for building the front-end templates, so it's definitely front-end-adjacent. Perhaps a conversation with @RLangeUni would be in order... or, if you'd like me to make some of my proposed changes so that you can test them, that's also an option (though I probably won't get to it quickly due to my long backlog of reviews at the moment). |
|
@RLangeUni has informed me about the current status of the PR. The ID is already provided and the access to the ID in the templates should be implemented in a separate PR. |
|
Thanks, @elsenhans -- then I'll just wait for the work to be completed on my earlier review comments. If I can do anything to help in the meantime, please let me know! |
|
@elsenhans / @RLangeUni, with the 10.0 release beginning to draw near, do you expect to do more work on this soon, or should we reschedule for a future release? I also notice that there is a conflict in the test class -- please let me know if you need my help to resolve that. |
* added basic exception for 'getUniqueIdWithSourcePrefix' method * changed 'getCheckbox' variable generation
* added basic exception for 'getUniqueIdWithSourcePrefix' method * changed 'getCheckbox' variable generation
jpkanter
force-pushed
the
pull-request-unique-html-id
branch
from
7c4a3f7 to
3d33b82
Compare
* added workaround for phpUnit consecutive test
|
As @RLangeUni is on vacation this week i quickly tried to take over the open points but my lack of knowledge with PHPUnit made it impossible to replace the correct withConsecutive Call. Is next week early enough? |
There was a problem hiding this comment.
@jpkanter, thanks for the progress here. I haven't fully reviewed your changes yet since I'm not sure if you're completely done yet, but I noticed that your test changes may be able to be simplified. See below...
| * @param ...$args | ||
| * @return callable | ||
| */ | ||
| public function consecutiveCalls(...$args): callable |
There was a problem hiding this comment.
You might find it helpful to use WithConsecutiveTrait.php here -- that's the general-purpose solution I used to replace the deprecated withConsecutive method in other tests.
There was a problem hiding this comment.
Thats an interesting headsup, i will look into it tomorrow. I am indeed not done, i quickly threw some code together as the necessary changes seemed rather simple and i had no running test-environment on my local machine. As i cannot continue to spam not working tests i should probably get that going again.
|
@jpkanter, this can definitely wait a week or two if necessary. But if you need help adapting the test to use WithConsecutiveTrait as I suggested, let me know and I don't mind helping. :-) |
| $context = compact('number') + [ | ||
| 'id' => $this->getUniqueIdWithSourcePrefix(), | ||
| 'checkboxElementId' => $this->getUniqueHtmlElementId($idPrefix) | ||
| ]; |
There was a problem hiding this comment.
Minor style point: I'd de-indent the closing ]; here for consistency with code style elsewhere.
…moved to some utils class anyway
…end - kind of weird phpstan proposal
|
I implemented a unique result set identifier for records, ensuring that each record receives a UUID-like identifier to avoid conflicts. PHPStn forced me to add the $recordCollection property in AbstractBackend. generateUuid() could be more handsome and moved to some Utils class? But I didn't found an existing appropriate. The template changes should not be merged. They are more for my own comparism. For example in result-list should be later changed like |
There was a problem hiding this comment.
Thanks, @RLangeUni! I think we're nearly done here, but I think the phpstan issue you reported may have been related to some orphaned and unnecessary code; see below for details.
| */ | ||
| public function setResultSetIdentifier(string $uuid) | ||
| { | ||
| $this->recordCollection->setResultSetIdentifier($uuid); |
There was a problem hiding this comment.
Is this method actually used? I think the property you were forced to add was related to the call in this method, but I suspect this method is unnecessary.
…ctBackend - set uuid only ON record or collection" This also reverts commit 48e5a10.
…ltSetIdentifier in methods like interface and use inheritdoc
…ged in following PRs
|
In the AbstractBackend class, I already had a slight suspicion that I was too eager. It seems "setResultSetIdentifier" is sufficient for the records/collections. Is the generation of the UUID alright? With Doctrine, one could also use its helper methods (Doctrine\ORM\Id\UuidGenerator). |
|
"injectSourceIdentifier" is also called from InjectSpellingListener.php - should we check within AbstractBackend::injectSourceIdentifier() whether |
I suppose it wouldn't hurt to count the records before generating the UUID to potentially save a few cycles, though I don't think it's a huge issue. |
| * | ||
| * @var string|null | ||
| */ | ||
| protected $resultSetIdentifier; |
There was a problem hiding this comment.
I'd suggest explicitly initializing this to null.
| "/\s+/", | ||
| '_', | ||
| ($idPrefix ? $idPrefix . '-' : '') | ||
| . $this->driver->getResultSetIdentifier() . '-' |
There was a problem hiding this comment.
We should add handling here to ensure this value is non-null, since concatenating null into a string may cause a notice. It would probably be good to add a case to the unit test to cover the "null record set identifier" case so we can ensure it behaves as expected.
| @@ -116,6 +119,27 @@ abstract public function getRecordCollectionFactory(); | |||
| protected function injectSourceIdentifier(RecordCollectionInterface $response) | |||
| { | |||
| $response->setSourceIdentifiers($this->identifier); | |||
| $response->setResultSetIdentifier($this->generateUuid()); | |||
There was a problem hiding this comment.
I think this approach is fine for now, but maybe it's worth putting a TODO comment here to switch to using the Doctrine UUID generator once it becomes available to us in the future. Then we could potentially address that as part of (or sometime after the merge of) #2233.
|
I'm also requesting a review from @maccabeelevine since #3552 is going to depend on the work here, so his opinion/input may be valuable! |
…null and concat to empty string
…null and concat to empty string
|
Adopted the last suggestions. Thanks for your support! |
There was a problem hiding this comment.
Thanks, @RLangeUni, this all looks good to me now. I appreciate your persistence in seeing this through to the end. :-)
I'm not going to merge it quite yet, just in case anyone else has last-minute feedback. I'll resolve it next week if there is no conversation before then.
|
Thanks, @RLangeUni and @maccabeelevine. @EreMaijala pointed out that it might be better to generate real UUIDs with the ramsey/uuid library to prevent confusion. I'm inclined to agree but don't want to further delay this PR, so I'll merge this as-is and open a separate UUID PR shortly. |
According to #2874 (comment), this PR is the foundation for using unique ids to reference to from other tags with "aria-describedby" or similar.
$idPrefix could be the modal or the context ('list-entry', 'holds', 'illrequests', 'storage-retrieval-requests', 'search-result'). Perhaps a second parameter in getUniqueHtmlElementId could be added to differentiate between context and prefix?
TODO: