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

[FEATURE] Register dashboards, widgetGroups and widgets by PHP API #105

Closed
wants to merge 4 commits into from

Conversation

haassie
Copy link
Collaborator

@haassie haassie commented Nov 26, 2019

Besides registering dashboards, widgetGroups and widgets by the Dashboard.yaml file, you can now register dashboards, widgetGroups and widgets by a PHP API as well.

An example how to use this API:

$newDashboard = GeneralUtility::makeInstance(\FriendsOfTYPO3\Dashboard\Configuration\Dashboard::class);
$newDashboard->setIdentifier('dashboard-new');
$newDashboard->setLabel('Label of dashboard');
$newDashboard->setDescription('Description of dashboard');

$newWidgetGroup = GeneralUtility::makeInstance(\FriendsOfTYPO3\Dashboard\Configuration\WidgetGroup::class);
$newWidgetGroup->setIdentifier('widgetGroup-new');
$newWidgetGroup->setLabel('Widget group');

$newWidget = GeneralUtility::makeInstance(\FriendsOfTYPO3\Dashboard\Configuration\Widget::class);
$newWidget->setIdentifier('widget-new');
$newWidget->setClassName(NumberOfAdminBackendUsersWidget::class);
$newWidget->setGroups(['widgetGroup-new']);

$dashboardConfiguration = GeneralUtility::makeInstance(\FriendsOfTYPO3\Dashboard\DashboardConfiguration::class);
$dashboardConfiguration->registerDashboard($newDashboard);
$dashboardConfiguration->registerWidgetGroup($newWidgetGroup);
$dashboardConfiguration->registerWidget($newWidget);

Resolves: #102

@sfsmfc
Copy link

sfsmfc commented Nov 27, 2019

@haassie Change looks good for me.

We have to add a new test for registering dashboard, widget and widgetgroup and update the documention for this part.

Copy link

@sfsmfc sfsmfc left a comment

Choose a reason for hiding this comment

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

Looks fine and works for me.

@NeoBlack
Copy link
Member

That was not the idea as I removed the WidgetRegistry and introduced the registration by the YAML file.
It is not a good idea to have multiple ways to register widgets. The YAML approach was to make it possible to cache the whole registration and improve the performance, btw. the same approach we use for other things like the routes configuration.
The configuration classes were designed to be immutable, that's the reason why it has no setter methods.
I am strongly against this patch and opt for not merging it.

@kaystrobach
Copy link
Contributor

@NeoBlack there is just one reason for having the php api, which would be making widgets available based on conditions (other installed extensions, and other stuff).

If that's possible with the yaml files or PageTS Settings, then I would also vote against this patch.

If there is no way to hide widgets / groups / dashboards based on conditions, I would argue to discuss that further and find a solution.

Caching for performance reasons is a very valid reason IMHO.

@haassie
Copy link
Collaborator Author

haassie commented Nov 27, 2019

That was not the idea as I removed the WidgetRegistry and introduced the registration by the YAML file.
It is not a good idea to have multiple ways to register widgets. The YAML approach was to make it possible to cache the whole registration and improve the performance, btw. the same approach we use for other things like the routes configuration.
The configuration classes were designed to be immutable, that's the reason why it has no setter methods.
I am strongly against this patch and opt for not merging it.

@NeoBlack Having a YAML-only approach is in my opinion not enough especially conditions is an important part for me. Conditions like extensions installed and ApplicationContext are just a few of the possible conditions you might have to register specific widgets or not.

Routes is another usecase if you ask me. It is not a problem when a route is registered but not used. If a widget is registered it can be used by a backend-user. Although you might fix that with permissions, it is not making things easier.

In my opinion YAML should be easy. If we add all kind of options and possibilities for conditions, we will lose that "easy" solution where integrators can register their own dashboards.

About caching: We still cache the configuration after it is generated. So what will be the problem there?

About the immutability you have a point. I can refactor that a little bit to remove the setters.

@kaystrobach
Copy link
Contributor

kaystrobach commented Nov 27, 2019

About caching: We still cache the configuration after it is generated. So what will be the problem there?

I think the main Problem with caching is that point, when we come to strange edge cases.
Conditions are not that easy to cache, as the condition result might change during the cache life time.

Just remember the strange ext_tables.php problems people had because of shitty conditions.

@NeoBlack
Copy link
Member

NeoBlack commented Nov 27, 2019

Having a YAML-only approach is in my opinion not enough especially conditions is an important part for me. Conditions like extensions installed and ApplicationContext are just a few of the possible conditions you might have to register specific widgets or not.

Conditions could be applied (as a new feature) with Symfony expression language, this would work also for cached configurations.

About caching: We still cache the configuration after it is generated. So what will be the problem there?
A condition based on PHP will be interpreted exactly one time, at the moment the cache is built.

This will result in an undefined and wrong state, also we lose the option for cache warmup.

Just remember the strange ext_tables.php problems people had because of shitty conditions.

Yes, exactly this is the reason why I don't want any PHP for configuration level.
Use PHP for dynamic expressions will result in a lot of problems.

@haassie
Copy link
Collaborator Author

haassie commented Nov 27, 2019

We first have to discuss this YAML vs PHP and what the default will be for TYPO3 core. I want to prevent that yaml files become too big (like EXT:form) and that we loose flexibility.

@NamelessCoder
Copy link
Contributor

Conditions could be applied (as a new feature) with Symfony expression language, this would work also for cached configurations.

This, and all other dynamic features, are natively supported by using PHP. The complexity of using YAML keeps increasing.

This will result in an undefined and wrong state, also we lose the option for cache warmup.

A cache warmup that you only need in the first place because YAML, not PHP, is being used.

In terms of performance, YAML will never beat PHP and I think it's an odd argument to use; that "YAML performs better" - there are many reasons to use YAML but performance is absolutely not one of them.

Use PHP for dynamic expressions will result in a lot of problems.

As discussed on the core mergers meeting today, using a thing like Symfony's configuration package would serve both of those purposes. It would seem the more reasonable solution is to simply wait until there is a uniform configuration API: we avoid the "NIH", and we get both PHP and YAML (and XML, and... and... and...). So I recommend we wait!

@haassie
Copy link
Collaborator Author

haassie commented Feb 25, 2020

The extension is now merged in core and we this repository won't be used anymore. This specific issue is already resolved in the core.

@haassie haassie closed this Feb 25, 2020
@haassie haassie deleted the feature/101-PHP-API branch February 25, 2020 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to register dashboards, widget groups and widgets by PHP
5 participants