Contrib.social

Feedback on code

Open on Drupal.org →
Created on 21 January 2025, 10 days ago

Problem/Motivation

Requested feedback on code and received super good feedback from Scott

Feedback

Some nitpicks/suggestions (take these with a grain of salt :slightly_smiling_face: )
JS: In the js file you split on ||, what if the response contains that? Maybe worth escaping response somehow?
Controller: There is Json::decode() method in core, maybe better the json_decode
Controller: You're accessing directly $_POST values, better to get from request object and make sure you are escaping
Search form: I believe you will have problems if 2 instances of the block are added to the page because of the submit wrapper attributes (could be wrong)
Search form: Probably worth making the results a template so people can theme it
Search block: Max age 0 probably needs to be a lazy builder callback instead or you'll break caching on entire page
Search block: Would suggest access check default to true
Search block: I believe you are relying on it being layout builder, but what if its just Structure > Block Layout?
Search block: Alternative to access check is to load a View instead and let the user do what they want there (e.g. they can then add their own filters). Snippet below
Converting back to HTML: You could use CommonMarkConverter from PHP League maybe? Then LLM can just return markdown which it might be more reliable at?
$view_id = Settings::get('ai_search_view_id');
$view = Views::getView($view_id);
$view->setDisplay('block_1');
$view->setExposedInput([
'search_api_fulltext' => $question,
]);
$view->execute();
if (!empty($view->result)) {
...

💬 Support request
Status

Active

Component

Code

Created by

🇧🇪Belgium wouters_f Leuven

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @wouters_f
  • Comment 10 days ago →
    🇨🇦Canada ScottEuser
  • Comment 10 days ago →
    🇧🇪Belgium wouters_f Leuven
  • Comment 10 days ago →
    🇧🇪Belgium wouters_f Leuven

    Processed most of these.

    1. split on a little omre complex character now (not sure if more complexity is needed)
    2. Json decode replaced
    3. POST vars replaced
    4. problems with 2 instances: that is correct. it's either locked to 1 block without conflicts (had this in the first version) or over two block with possible conflicts. Currently this is how I want to do it. might change later
    5. worth making the results into a template: will make a separate issue for that
    6. max age 0 and cache dissabling removed (was for development)
    7. layout builder references removed, we are not dependant on layout builder at all
    8. added a select box for the permission check. I hear others want other permission systems too, so we'll have the choice to change later
    9. Will make a separate issue for views access checking of filtering
    10. Converting back to HTML: also implementing this,
  • Comment 4 days ago →
    🇧🇪Belgium wouters_f Leuven

    Most of these were implemented.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024