apigee-edge-drupal icon indicating copy to clipboard operation
apigee-edge-drupal copied to clipboard

Use FieldAttributeConverterInterface instead of concrete FieldAttributeConverter class in constructor injection

Open berramou opened this issue 1 month ago • 1 comments

Is your feature request related to a problem? Please describe.

Yes. Currently, the \Drupal\apigee_edge\Form\DeveloperAttributesSettingsForm class injects the concrete class \Drupal\apigee_edge\FieldAttributeConverter in its constructor.

Since FieldAttributeConverter is declared final, it cannot be extended. This causes a TypeError when another module tries to decorate or override the service apigee_edge.converter.field_attribute, because the decorated service is not an instance of the final class.

Example error:

TypeError: Argument #4 ($field_attribute_converter) must be of type 
Drupal\apigee_edge\FieldAttributeConverter

This limits extensibility and makes it difficult for other modules to customize or wrap the service.

Describe the solution you would like

Update DeveloperAttributesSettingsForm constructor type hints to use the interface FieldAttributeConverterInterface instead of the concrete class FieldAttributeConverter.

public function __construct(ConfigFactoryInterface $config_factory, EntityFieldManagerInterface $entity_field_manager, FieldStorageFormatManagerInterface $field_storage_format_manager, FieldAttributeConverterInterface $field_attribute_converter, TypedConfigManagerInterface $typed_config_manager) {
    parent::__construct($config_factory, $typed_config_manager);
    $this->fieldAttributeConverter = $field_attribute_converter;
    $this->entityFieldManager = $entity_field_manager;
    $this->fieldStorageFormatManager = $field_storage_format_manager;
  }

This change will:

  • Allow other modules to decorate or replace the service without type conflicts.
  • Follow the dependency inversion principle.
  • Improve extensibility and maintainability.

Describe alternatives you have considered

  1. Describe alternatives you have considered
  2. Using a service decorator — fails because FieldAttributeConverter is final
  3. Removing the type hint in the constructor — works but reduces type safety and is not ideal
  4. Replacing the service entirely — possible but risky, as it may break other parts of the module relying on the original implementation.
  5. Using the interface is the cleanest, most future-proof solution.

Additional context

Example use case: A custom module wants to extend FieldAttributeConverter (e.g., to add custom logic or logging). However, because the class is final and type-hinted directly, the decorated service cannot be injected into the form.

Updating the constructor to depend on FieldAttributeConverterInterface would fix this and allow safe decoration.

berramou avatar Nov 01 '25 18:11 berramou

@berramou , Thank you for bringing this issue to our attention and sharing a corresponding fix. We will review your Pull Request and follow up shortly.

shishir-intelli avatar Nov 03 '25 12:11 shishir-intelli

@berramou

Thank you for this contribution. I have reviewed PR #1199 and tested it locally. The code itself looks good.

However, I identified some phpcs-related issues that need to be addressed. Could you please fix these coding standards violations?

If you run into any difficulty, you can refer to this patch where I have resolved the issues.

shailesh-google avatar Nov 07 '25 11:11 shailesh-google

Hello @shailesh-google Fixed here https://github.com/apigee/apigee-edge-drupal/pull/1199/commits/a21d7b8795ccd9fa02818ca2d6cb91f92cd9da28

berramou avatar Nov 07 '25 12:11 berramou