Add address field automator type

Hi, thanks and cool - is the idea basically to look through unstructured texts that has full addresses in them somewhere? I think if so, its important to add somewhere that it should only store the information that it can see (I added a comment about).

There is a similar field in https://www.drupal.org/project/google_places → , that can take partials and verify against Google Places API, but that requires one extra API key of course.

Hi, Yes it's exactly the use case : get unstructured result (or any non structured contribution) and fill structured address.
I should definitely both refactor the prompt based on field configuration and add post process check for required fields.
Google places check could be an optional step also.
Thank you.

Added validation and dynamic address fields from field config.

The code doesn't introduce any security issue, good.

However:

As pointed out by @MrDaleSmith, there is duplicity in:

public function runChatMessage(string $prompt, array $automatorConfig, $instance, ?ContentEntityInterface $entity = NULL) {
  $text = $this->runRawChatMessage($prompt, $automatorConfig, $instance, $entity);
  // Normalize the response.
  $json = $this->promptJsonDecoder->decode($text);
  if (!is_array($json)) {
    throw new AiAutomatorResponseErrorException('The response was not a valid JSON response. The response was: ' . $text->getText());
  }
  return $this->promptJsonDecoder->decode($text);
}

Which should probably be fixed by:

$json = $this->promptJsonDecoder->decode($text);
if (!is_array($json)) {
  throw new AiAutomatorResponseErrorException('Invalid JSON: ' . $text->getText());
}
return $json;

There also are repeated class_exists() checks:

$addressFieldOverrideClass = '\CommerceGuys\Addressing\AddressFormat\FieldOverride';
if (!class_exists($addressFieldOverrideClass)) {
  return [];
}

Again as pointed by @MrDaleSmith, these should be in a single method, e.g. addressClassesArePresent(), that checks for all needed \CommerceGuys\Addressing\... classes at once.

Lastly, there is potential confusion with array_merge_recursive() in generate() - please check if it does what is expected: if LLM returns the same keys, array_merge_recursive() can cause unexpected nesting. For example, if $values has keys that already exist in $total, it may end up generating nested arrays instead of appending in a simple manner.

@MrDaleSmith & @seogow You're right.

The classes existence check should be dedup.
This all could be replaced by a module handler module exists call for address module and located in the "ruleIsAllowed()" method.
I cannot just focus on "field type id" in the plugin definition as nothing really enforce other module to declare "address" field type.
Also set "class names strings" as constant variables in the plugin class should be done.

To be honest, in my opinion, this plugin should part of the address module by itself as a "extra feature" of the address module. But also here, the dependencies with uses classes for inheritance may be tricky as the ai module should not be required by the address module.

I'll check also the array_merge_recursive point.

Thanks a lot for your feedback

Comments fixed.
I didn't add the check before array_merge_recursive because it would duplicate the verify.
The verify could directly be called by runChatMessage but the check wont be in the right place regarding AiAutomatorRuleRunner::generateResponse implementation.

Changes all good. Tested the new plugin and it correctly found the address and set it in an address field - managed to get the full address from a mention of "10 Downing Street, SW1A 2AA" in the text which is good (but might suggest it's augmenting the addresses from its own training data, but I think we should cross that bridge if we reach it).

Sorry, for setting it back in Needs Work, but there are two details that it would be good if you could look at.

Also I think it would it make sense to add commerceguys/addressing as a suggest in the composer.json with a description of what it does?

OK @markus_johansson I take care of it.

Classes based on "string names" is related to the fact that another module apart from address module may declare "address" field type.

But as we check that address module (which is unique) is enabled and the fact that it requires commerceguys/addressing we are good with the use of ruleIsAllowed() here.
So the UX will be consistent : no automation on "address" fields that would come from another module than "address". And the ai module won't crash trying to instantiate the Address automator because it does not "uses" any class from outside (coming from commerceguys/addressing and address module).

This is also why I didn't added the commerceguys/addressing as it is a requirement of the address module this automator depends on.

Thanks mattic - I have done functional review and it works well! I added three comments with suggestions though - the first one should be done before merge, since we have to support PHP 8.1. The other two you can add if you think the code gets cleaner.

If the first is added and decide on the other two and I'll merge.

I saw that the library is part of the address module, so no need to have to add it to suggest.

Thank you Marcus, Just pushed fixes.

Thank you mattic - getting merged and released in the next release.

Automatically closed - issue fixed for 2 weeks with no activity.