- Issue created by @michaellander
- Merge request !512Split out appliesToContextDefinition method and added additional token support... β (Open) created by michaellander
- πΊπΈUnited States michaellander
With this branch, the workflow is:
- When
FunctionCall::setContextValue()
is called, loop throughAiContextConverter
plugins and checkapplies()
- If found(
TRUE
), stop looping and callconvert()
- If none apply, pass original value
Where this gets a bit tricky is to what extent
applies()
should evaluate in regards to returning TRUE/FALSE. Should it just match the proper format? should it be able to successfully convert without doing the conversion?Let's say we are given 'node:17', we can confirm the first part of the string contains a valid entity type, we can confirm the second part is a real entity. However, if entity type is correct, but the entity doesn't exist, do we return FALSE and let it continue checking for a match? One thing we may want to decide is if the
applies
call should remain binary or provide more potential states(think\Drupal\Core\Access\AccessResult
). We'd create aAiConverterAppliesResult
class, with methods such as:applicable
,invalidApplicable
,notApplicable
. This would give us the ability to differentiate converters that definitely don't apply vs ones that may apply but theres an issue(i.e.InvalidApplicable('Node loaded but translation "fr" does not exist')
) vs converters that will apply and succeed.This means the workflow could evolve to:
- When
FunctionCall::setContextValue()
is called, loop throughAiContextConverter
plugins and checkapplies()
- If
applicable
found, convert - If
invalidApplicable
, return these with description on why they were invalid - If
notApplicable
, fail, and decide what to do
I like this approach better because it's up to the implementer to decide how to treat success, partial success and failure. They could still interpret them as true/false, or they can potentially take the natural language description and send that back to a provider and even prompt the user for clarification on how to move forward.
I just want to maybe get some other eyes on it before I continue.
- When
- πΊπΈUnited States michaellander
michaellander β changed the visibility of the branch 3512100-context-upcaster_applies-class to hidden.
- πΊπΈUnited States michaellander
I marked 'Needs review' but really looking for feedback before I go any further. I may have not used that status correctly!
- πΊπΈUnited States michaellander
I'm wondering if the DataType up casting/converting should be independent of Contexts...I may be unnecessarily relating them.. I'm going to go back through this and see if that's the case.
- π¬π§United Kingdom MrDaleSmith
Code all looks good an applies cleanly, but as this is such an under-the-hood change I'm finding it hard to reassure myself it has passed testing. Can you maybe suggest a test or two that would confirm it's working as expected?
- π©πͺGermany marcus_johansson
I'm going through the AI issue queue and for me this looks good now, would you be ok with us merging it @michaellander?