Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add enum support for DCAs and models #6584

Merged
merged 6 commits into from
Jan 9, 2024

Conversation

SeverinGloeckle
Copy link
Contributor

Implements #6546

A few notes on the implementation:

1. Usage of TranslatableMessage

The generation of translated references for the generated options is implemented via Symfony’s TranslatableMessage.

In my opinion, this is the most sane way to handle translations in a complex system with various sources of translation.

Using Symfony’s TranslatableInterface however is less restrictive, but does not provide the additional information on parameters and especially on the translation domain.

Although it is outside the scope of this PR, a forward-looking decision ought to be made on how Contao wants to deal with translations in the future. Up to now, neither of the two interfaces is being used.

2. Validation of enum values in the model

It’s not straightforward to define a proper handling of invalid enum values in the model. One could:

  • simply return null for invalid values
  • raise an exception explaining the invalid value (like implemented)
    • Since there is no built-in way to detect whether an enum is backed by strings or integers, only a generic exception for both types is possible (without adding somethin like an EnumUtil).
  • let the call to BackedEnum::tryFrom() fail with the invalid value and let it throw its TypeError

I implemented the second way in order to give developers a proper hint pointing to the DCA configuration but the other two ways are reasonable as well.

3. Modifications & unit tests for legacy code

For the changes to the legacy code, I stuck to existing styles and structures as much as possible.

For the model, only a simple unit test is added for resolving the enum - if necessary, I can also add tests for the various exceptions.

4. Needs documentation

If this will be merged, I'll also add corresponding notes to the developer documentation.

@aschempp
Copy link
Member

Any particular reason we need the DcaExtractor to read the enums? Why not just use the listener to convert enum to string options?

@Toflar Toflar requested a review from a team November 30, 2023 10:21
@SeverinGloeckle
Copy link
Contributor Author

The DcaExtractor maps the individual fields to the configured enum classes and transfers this assignment to the model.

To resolve the enums via Model::getEnum(), the specific enum class is required in addition to the value of the field (see example in third section of #6546)

@leofeyer leofeyer added this to the 5.3 milestone Nov 30, 2023
@leofeyer leofeyer linked an issue Dec 14, 2023 that may be closed by this pull request
leofeyer
leofeyer previously approved these changes Dec 28, 2023
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that the code works. 👍

@leofeyer
Copy link
Member

@aschempp Has your question above been answered?

@contao/developers Shall we add two example enums so people can use them as templates for their own enums?

namespace Contao\CoreBundle\DataContainer\Enum;

enum PageRobotsEnum: string
{
    case indexFollow = 'index,follow';
    case indexNofollow = 'index,nofollow';
    case noindexFollow = 'noindex,follow';
    case noindexNofollow = 'noindex,nofollow';
}
namespace Contao\CoreBundle\DataContainer\Enum;

use Contao\CoreBundle\Translation\TranslatableLabelInterface;
use Symfony\Component\Translation\TranslatableMessage;

enum PageRedirectEnum: string implements TranslatableLabelInterface
{
    case permanent = 'permanent';
    case temporary = 'temporary';

    public function label(): TranslatableMessage
    {
        return new TranslatableMessage('tl_page.'.$this->name, [], 'contao_tl_page');
    }
}

Do you agree with the namespace?

@Toflar
Copy link
Member

Toflar commented Dec 28, 2023

@SeverinGloeckle any reason you did not go for 'options' -> Enum but a new key instead?

@SeverinGloeckle
Copy link
Contributor Author

@contao/developers Shall we add two example enums so people can use them as templates for their own enums?

enum PageRobotsEnum: string
enum PageRedirectEnum: string implements TranslatableLabelInterface

The two examples are a very good idea. However, I would prefer to add them in a follow-up PR for an independent discussion (namespace, extensibility, possibly resolution within the PageModel).

I may create a follow-up PR as soon as this PR has been merged.

@SeverinGloeckle
Copy link
Contributor Author

@SeverinGloeckle any reason you did not go for 'options' -> Enum but a new key instead?

From my point of view, the explicit tagging of the enum provides the following benefits:

  1. unambiguous check whether a DCA field is linked to an enum (no type check)
  2. generation of references even if the options were overwritten manually (see 90f9e22)
  3. no side effects on third party code (which may rely on options being an array)

Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with that

@leofeyer leofeyer requested a review from ausi January 4, 2024 10:52
@leofeyer leofeyer changed the title Enum support for DCAs & models Add enum support for DCAs and models Jan 9, 2024
@leofeyer leofeyer merged commit 51138dc into contao:5.x Jan 9, 2024
16 checks passed
@leofeyer
Copy link
Member

leofeyer commented Jan 9, 2024

Thank you @SeverinGloeckle.

leofeyer pushed a commit that referenced this pull request Apr 25, 2024
Description
-----------

**Problem:**
The `DcaExtractor` is parsing DCA data and provides information about any enumeration fields within a DCA (see #6584 for the original feature). However, if the extracted data is cached by the `ContaoCacheWarmer`, the enum configuration is missing.

This results in an incorrect/incomplete configuration and it is not possible to resolve enumerations within a model if the extract is read from the cache.

**Solution:**
This PR adds the enumeration configuration to the cached data in `ContaoCacheWarmer`, making the information available in cached extracts.

**Additional optimisation & Tests**
Manually assembling the extract for the cache does not appear to be the most sustainable solution. However, this is outside the scope of this PR.

I have not made any adjustments to the tests in `ContaoCacheWarmerTest`, as so far only the output of `$this->arrFields` is tested in the extract and any other information (e.g. `arrRelations`, `arrKeys`, ...) is not checked explicitly.

If an addition to the test is desired, I can extend the test case and test data to include a field with an enum configuration.

Commits
-------

407b777 Fix DCA extractor cache for enum fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

First class enum support in DCA option fields
5 participants