[MGNLUI-3725] Reconsider JCR-specific code in LinkFieldFactory's choose-dialog callback Created: 11/Dec/15  Updated: 11/Jan/16  Resolved: 06/Jan/16

Status: Closed
Project: Magnolia UI
Component/s: forms
Affects Version/s: 5.4.3
Fix Version/s: 5.4.4

Type: Improvement Priority: Major
Reporter: Zdenek Skodik Assignee: Christoph Meier
Resolution: Fixed Votes: 0
Labels: linkfield, support
Remaining Estimate: 0d
Time Spent: 1d 1h
Original Estimate: Not Specified

Issue Links:
Relates
relates to MGNLUI-3683 Make LinkField a pure-Vaadin field, m... Closed
supersession
supersedes MGNLUI-3232 Implement LinkField that is not depen... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:
Sprint: Basel 25
Story Points: 5

 Description   

LinkFieldFactory$ChooseDialogCallback#onItemChosen is fishy at best. In particular, LinkFieldDefinition's targetPropertyToPopulate property is unclear, undocumented, most likely unused and should be deprecated. It strongly looks like legacy code pre-transformers and pre-identifierToPathConverter—which I'd expect to be used in that callback method instead.

At the moment, plain LinkFieldDefinition cannot be used with resources app in 5.4, because of this.

As a workaround, one may use a custom LinkFieldFactory for resources app, as shown at https://git.magnolia-cms.com/projects/modules/repos/resources/commits/1e662634b1243f44242d9f321ca9a75a1354e30b



 Comments   
Comment by Mikaël Geljić [ 18/Dec/15 ]

cmeier, zdenekskodik,

I'm under heavy suspicion that UI's LinkFieldFactory$ChooseDialogCallback#onItemChosen is fishy at best. In particular I'm not aware of any usage of LinkFieldDefinition's targetPropertyToPopulate property. Are you guys aware of any? To me, it looks like legacy code pre-transformers and pre-identifierToPathConverter—which I'd expect to be used in that callback method.

As such, it's not addressed by MGNLUI-3683 (at least that was not the intent, and this part is unmodified in the PR).
I believe we can clean this part under maintenance, instead of providing the custom field (which every other non-JCR app would have to provide as well).

I'll move that to MGNLUI if I may, and update title/desc. Let me know if I overlooked anything!

Comment by Christoph Meier [ 06/Jan/16 ]

Since this ticket gets resolved in the context of a minor release, i’ve tried to find a simple solution - following hads advice as well as zdenekskodik proposal.

The „fix“ on ChooseDialogCallback#onItemChosen now also handles choosenValue objects which are NOT of type JcrItemId.
To keep it simple, i handle types String and Object; in the latter case, the new value is just choosenValue.toString(). These two new cases should be sufficient for many cases which required a custom FieldFactory before.
I have discarded the initial idea to simplify the code also for the if (chosenValue instanceof JcrItemId) case. i don't know a case where targetPropertyToPopulate is used. But i cannot be sure that customers aren't using it as well.

Further changes in the "future"?
Another approach proposed by mgeljic suggests changing LinkField in a way that its value would be an Object instead of String. This would require more changes and could be a feasible improvement for a major version. The „start“ of this approach is available on a branch on my fork. The branch is incomplete and the LinkField requires a "default converter" (which was not added yet on the branch).
To end up with a real JCR-agnostic LinkField, its definition class LinkFieldDefinition also must be refactored (by removing targetWorkspace).

Comment by Christoph Meier [ 07/Jan/16 ]

feature/MGNLUI-3725 has been squashed. It is ready to integrate.

Comment by Mikaël Geljić [ 08/Jan/16 ]

QA'd with resources app

Generated at Mon Feb 12 09:09:28 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.