Problem/Motivation

Currently tool configuration is scattered across multiple entity properties — enabled_tools, locked_tools, and tool_operations — which makes the model harder to reason about, more error-prone and less type-safe. Maintaining parallel arrays introduces duplication, increases the chance of inconsistent state, and complicates validation, serialization, and change-tracking.

Proposed consolidation: move every tool's configuration into a single tools property on the entity where each tool is represented by one complete configuration object/array:

[
  'name' => 'tool_name',
  'description' => 'Tool description',
  'input_schema' => [...],        // MCP tool schema

  // Configuration
  'enabled' => true,
  'locked' => false,
  'operation' => 'read',          // read|write

  // Locked definition (only if locked === true)
  'locked_definition' => [
    'name' => 'tool_name',
    'description' => 'Tool description',
    'input_schema' => [...],
  ],
]

Consolidation creates a single source of truth and improves validation, maintainability, and type safety. It also prepares the codebase for properly typed DTOs/value objects.

Steps to reproduce

  1. Create an entity with the current model using enabled_tools, locked_tools and tool_operations.
  2. Perform an operation requiring tool state validation (e.g. checking enabled/locked/operation mode).
  3. Observe that logic must reconcile multiple arrays, increasing risk of inconsistent state.
  4. Attempt to add or modify tool metadata; duplication across arrays complicates maintenance.

Proposed resolution

Replace the scattered arrays with a single tools list on the entity and introduce typed DTOs/Value Objects for clarity and type safety:

  • ToolCollection — collection wrapper for all tools.
  • Tool — represents a single tool (config + metadata).
  • InputSchema — representation of the tool’s input schema.
  • LockDefinition — metadata snapshot when a tool is locked.

Example DTO API:

// ToolCollection
get(string $name): ?Tool
add(Tool $tool)
remove(string $name)
toArray()/fromArray()

// Tool
getName(), getDescription(), isEnabled(), isLocked(), getOperation()
withEnabled(), withLocked(), withOperation(), withLockedDefinition()
toArray()/fromArray()

Key behaviours:

  • When locked === true, a locked_definition must be present.
  • Validation occurs in constructors/factories.
  • Entity stores tools as a structured JSON or equivalent type.

Remaining tasks

  1. Design and implement DTOs (Tool, ToolCollection, InputSchema, LockDefinition).
  2. Create a migration script to consolidate old arrays into the new tools structure.
  3. Update entity schema: add tools property and deprecate old fields.
  4. Refactor logic to use DTO-based API instead of raw arrays.
  5. Write full validation and serialization tests.
  6. Update documentation and API references.
  7. Add getters/setters on the entity for ToolCollection.

User interface changes

Admin / configuration UI updates:

  • Replace multiple tool-related UI fields with a single consolidated tool configuration table.
  • Display locked tools with read-only metadata pulled from locked_definition.
  • Allow editing of a full tool object via a structured UI.

Migration UI:

  • Optional: provide a migration preview showing old vs new tool definitions.

API changes

New entity methods:

  • getTools(): ToolCollection
  • setTools(ToolCollection $tools)
  • getTool(string $name): ?Tool
  • hasTool(string $name): bool

Legacy methods become deprecated but remain readable for one release cycle.

Example JSON shape:

{
  "tools": [
    {
      "name": "tool_name",
      "description": "Tool description",
      "input_schema": { },
      "enabled": true,
      "locked": false,
      "operation": "read",
      "locked_definition": null
    }
  ]
}

Data model changes

Introduce a single tools field (JSON/document/list-of-maps). Remove the following legacy fields:

  • enabled_tools
  • locked_tools
  • tool_operations

Migration strategy:

  1. Read existing arrays and merge by tool name.
  2. Derive enabled, locked, and operation values.
  3. If locked, generate a proper locked_definition.
  4. Write consolidated structures into the new tools field.
  5. Deprecate old fields and remove after client migration period.

Issue fork mcp_client-3561170

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

robertoperuzzo created an issue. See original summary.

robertoperuzzo’s picture

robertoperuzzo
Primary language Italian
Location 🇮🇹 Tezze sul Brenta, VI
commented
Related issues: +#3559869: Set tool operation per tool, +#3537086: Lock title and descriptions
harivansh’s picture

harivansh commented
Assigned: Unassigned » harivansh

harivansh opened merge request !10

harivansh’s picture

harivansh commented
Assigned: harivansh » Unassigned
Status: Active » Needs review
robertoperuzzo’s picture

robertoperuzzo
Primary language Italian
Location 🇮🇹 Tezze sul Brenta, VI
commented

@harivansh I just fixed coding standards and unit tests. The DTO classes work properly! Thank you very much.

robertoperuzzo’s picture

robertoperuzzo
Primary language Italian
Location 🇮🇹 Tezze sul Brenta, VI
commented

Hi @harivansh,

I was wondering why you used the "wither" methods in Tool.php.

So, we know that using this pattern, the benefits are:

  • Immutability: Since the Tool is declared as final with readonly properties, the object cannot be modified after creation. The with*() methods allow you to create modified copies while preserving the original object intact.
  • Safe State Changes: Instead of mutating the object directly (which would cause errors with readonly properties), you get a new instance with the desired changes: $updatedTool = $tool->withEnabled(true); // Returns new Tool, original unchanged
  • Method Chaining: You can chain multiple transformations fluently:
    $newTool = $tool->withEnabled(true)
                       ->withOperation($operation)
                       ->withLocked(true, $definition);
  • Thread Safety: Immutable objects are inherently thread-safe since their state cannot change after construction, preventing concurrency issues.
  • Predictable Behaviour: Functions receiving a Tool object can't accidentally modify it, making code more predictable and easier to debug.

But the drawbacks are:

  • Memory Overhead: Each with*() call creates a complete new object. If you chain multiple changes, you create intermediate objects that immediately become garbage:
    $tool->withEnabled(true)->withOperation($op)->withLocked(true, $def); // Creates 3 Tool objects, only the last one is used
  • Performance Cost: Creating new objects is more expensive than mutating properties. With 7 constructors parameters, each wither method instantiates a new object copying all properties.
  • Verbose Boilerplate: Notice how repetitive the wither methods are - each manually passes all 7 parameters. Adding a new property requires updating all wither methods:
          public function withEnabled(bool $enabled): self {
         return new self($this->name, $this->description, $this->inputSchema,
                         $enabled, $this->locked, $this->operation, $this->lockedDefinition);
       }
  • Deep Copy Issues: The Tool contains nested objects (InputSchemaInterface, LockedDefinitionInterface). These are shared by reference between copies, so they're not truly independent if those nested objects are mutable.
  • Harder Debugging: When tracking down where an object state came from, you need to trace through potentially many object creations rather than following direct mutations.
  • Less Intuitive for Teams: Developers unfamiliar with immutability patterns might create objects expecting mutation to work, leading to bugs where changes appear to "disappear."

So, I would like to understand if we are forced to use with* methods or use a different approach like this:

Builder Pattern

Create a mutable builder that constructs the immutable object only once:

     final class ToolBuilder {
       private string $name;
       private string $description;
       private InputSchemaInterface $inputSchema;
       private bool $enabled = FALSE;
       private bool $locked = FALSE;
       private ?ToolOperation $operation = NULL;
       private ?LockedDefinitionInterface $lockedDefinition = NULL;

       public static function fromTool(Tool $tool): self {
         $builder = new self();
         $builder->name = $tool->name;
         $builder->description = $tool->description;
         // ... copy all properties
         return $builder;
       }

       public function setEnabled(bool $enabled): self {
         $this->enabled = $enabled;
         return $this;
       }

       public function setOperation(ToolOperation $operation): self {
         $this->operation = $operation;
         return $this;
       }

       public function build(): Tool {
         return new Tool($this->name, $this->description, /* ... */);
       }
     }

     // Usage: Only ONE object created
     $newTool = ToolBuilder::fromTool($tool)
       ->setEnabled(true)
       ->setOperation($op)
       ->setLocked(true, $def)
       ->build();
marcus_johansson’s picture

marcus_johansson commented
Status: Needs review » Reviewed & tested by the community

Works well, I have tested different setups. Also code wise I did do two comments, but they are NITs and can be changed later.

So ready to merge.

#3562654: Add a disable/enable all tools checkbox and #3537086: Lock title and descriptions are nice to have, but I think we could release an alpha with this and start thinking about error handling we msised.

marcus_johansson’s picture

marcus_johansson commented
Status: Reviewed & tested by the community » Fixed

Thank you all, getting merged.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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