External dependency injection system #15266

Tithugues · 2019-05-11T21:23:16Z

Description

What do you think about this kind of DI system please?

The current system seems to be restrictive: the definition of services should be done in PHP (which doesn't seem very readable for me), also the parameters can't be defined at the beginning (only when getting the object).

With this system, we defined the template class and it can be passed everywhere quite easily, and this may be done for other classes. Also in a second step, this would enforce the principle to use injection instead of building objects in other objects. (I would love to split the current Response class in HtmlResponse and AjaxResponse classes with a common ResponseInterface.)

Fixes #

Before submitting pull request, please review the following checklist:

Make sure you have read our CONTRIBUTING.md document.
Make sure you are making a pull request against the correct branch. For example, for bug fixes in a released version use the corresponding QA branch and for new features use the master branch. If you have a doubt, you can ask as a comment in the bug report or on the mailing list.
Every commit has proper Signed-off-by line as described in our DCO. This ensures that the work you're submitting is your own creation.
Every commit has a descriptive commit message.
Every commit is needed on its own, if you have just minor fixes to previous commits, you can squash them.
Any new functionality is covered by tests.

MauricioFauth · 2019-05-11T21:56:29Z

I would love to split the current Response class in HtmlResponse and AjaxResponse classes with a common ResponseInterface.

Great idea. As I said in #15248 (comment), I also want to improve how we handle request and response with a HTTP library.

Tithugues · 2019-05-12T13:05:52Z

If you find any bug to fix or any code to improve, please tell me, I'll do my best to do it.

Tithugues · 2019-05-12T19:49:57Z

Hi,
Do you please know how to get rid of the errors thrown in https://travis-ci.org/phpmyadmin/phpmyadmin/jobs/531442853 ?

Undefined variable: $containerBuilder

This variable is defined in common.inc.php which is called at the beginning of all files which have the error. 😕

Thank you.

MauricioFauth · 2019-05-14T15:40:19Z

This variable is defined in common.inc.php which is called at the beginning of all files which have the error. 😕

The static analyzer doesn't know about variables defined in another files. We have been ignoring these errors.

phpmyadmin/phpstan.neon.dist

Line 18 in 720e302

    
           - '#Undefined variable: \$(cfg|db|table|url_query|text_dir|server|pmaThemeImage|action|sql_query|lang|export_type|err_url|collation_connection|filename_template|transform_key|cn|goto|partitionDetails|token_mismatch|auth_plugin|username|hostname|display_query|where_clause)#'#Others

MauricioFauth · 2019-05-14T15:53:02Z

Some ways to solve this would be to get the variable some other way, or if it is a global variable, declare it in the file.

Tithugues · 2019-05-14T18:37:28Z

Some ways to solve this would be to get the variable some other way, or if it is a global variable, declare it in the file.

Yes, I know this is done for other variables. But doing this would encourage use of the container through the global variable in many places, whereas the idea would rather be to provide the data as an input.
So if this is OK for you, I would prefer to fill the phpstan.neon.dist file.

MauricioFauth · 2019-05-14T18:40:20Z

So if this is OK for you, I would prefer to fill the phpstan.neon.dist file.

No problem.

Tithugues · 2019-05-14T20:21:13Z

Please don't merge it for now, I've an error after the creation of a table.

Tithugues · 2019-05-14T20:34:21Z

Please don't merge it for now, I've an error after the creation of a table.

Please ignore my previous message, it was on the other PR I'm working on.

MauricioFauth · 2019-05-14T21:43:39Z

ajax.php

-    $GLOBALS['PMA_Config']
-);
+/** @var AjaxController $controller */
+$controller = $containerBuilder->get('ajax_controller');


What do you think about using AjaxController::class instead of 'ajax_controller'?

This is OK for me. I'll rename the controller services with the class name of the controllers.

codecov · 2019-05-17T21:13:11Z

Codecov Report

Merging #15266 into master will decrease coverage by 0.02%.
The diff coverage is 10.57%.

@@             Coverage Diff              @@
##             master   #15266      +/-   ##
============================================
- Coverage     52.82%    52.8%   -0.03%     
  Complexity    14059    14059              
============================================
  Files           484      484              
  Lines         62163    62198      +35     
============================================
+ Hits          32840    32841       +1     
- Misses        29323    29357      +34

williamdes

Good work!

db_events.php

db_multi_table_query.php

db_central_columns.php

williamdes · 2019-05-18T14:32:13Z

server_databases.php


 if (! defined('ROOT_PATH')) {
    define('ROOT_PATH', __DIR__ . DIRECTORY_SEPARATOR);
 }

 require_once ROOT_PATH . 'libraries/common.inc.php';

+$response = $containerBuilder->get('response', ContainerInterface::NULL_ON_INVALID_REFERENCE) ?? Response::getInstance();


Can you use ::class ?

As I defined response as a service in the other PR, I would like to do it in the other one if this is OK for you.

MauricioFauth

Excellent work!

MauricioFauth · 2019-05-18T16:19:25Z

@Tithugues Do you think it's now possible to remove our custom DIC (PhpMyAdmin\Di\Container)?

Tithugues · 2019-05-18T19:22:49Z

@Tithugues Do you think it's now possible to remove our custom DIC (PhpMyAdmin\Di\Container)?

Not yet. But I should confess this is one on my aims…
I think we should go further before this. Your current Container has one advantage that I would like not to implement with the new one: getInstance. Thanks to this, you can retrieve every service everywhere. But if we achieve to inject dependencies instead of creating objects, we should be able to remove the current container, I think.
(This is a bit confusing because I think that I'm currently doing 2 things at the same time: defining the services externally thanks to the Symfony DI + injecting dependencies.)
The first things I'm currently trying to get rid of the current container are:

registering every old services as a service with the new container;
replacing as much as possible the get of services by a get through the new container.

When all the get will be replaced, it won't be fully finished, because there is also some magic in your container that loads the parameters regarding name and/or type (don't really remember, I had a quick look) of registered services.

db_datadict.php

williamdes

💯

Tithugues · 2019-05-18T23:25:40Z

Do you want me to squash the commits before merging the branch?

MauricioFauth · 2019-05-18T23:45:48Z

You can do it, if you want.

Signed-off-by: Hugues Peccatte <hugues.peccatte@gmail.com>

Tithugues · 2019-05-18T23:55:22Z

Done.

MauricioFauth · 2019-05-19T03:59:05Z

There are a few Selenium errors. You can open the link in the log to see the video of the test.

https://travis-ci.org/phpmyadmin/phpmyadmin/jobs/534311833#L494

Signed-off-by: Hugues Peccatte <hugues.peccatte@gmail.com>

Tithugues · 2019-05-19T13:40:15Z

There are a few Selenium errors. You can open the link in the log to see the video of the test.

https://travis-ci.org/phpmyadmin/phpmyadmin/jobs/534311833#L494

Thanks, I did some fixes. I'll wait for Selenium feedback.

MauricioFauth · 2019-05-19T14:56:51Z

Excellent work! Thank you for your contribution.

External dependency injection system [ci skip] Signed-off-by: Maurício Meneghini Fauth <mauricio@fauth.dev>

External dependency injection system [ci skip] Signed-off-by: Maurício Meneghini Fauth <mauricio@fauth.dev> Signed-off-by: Nupur Agrawal <nupur202000@gmail.com>

MauricioFauth approved these changes May 11, 2019

View reviewed changes

Tithugues marked this pull request as ready for review May 12, 2019 13:01

MauricioFauth reviewed May 14, 2019

View reviewed changes

williamdes requested changes May 18, 2019

View reviewed changes

db_events.php Outdated Show resolved Hide resolved

db_multi_table_query.php Outdated Show resolved Hide resolved

williamdes reviewed May 18, 2019

View reviewed changes

db_central_columns.php Outdated Show resolved Hide resolved

williamdes requested changes May 18, 2019

View reviewed changes

MauricioFauth approved these changes May 18, 2019

View reviewed changes

williamdes reviewed May 18, 2019

View reviewed changes

db_datadict.php Outdated Show resolved Hide resolved

williamdes approved these changes May 18, 2019

View reviewed changes

Define controllers as services in an external configuration file

380707f

Signed-off-by: Hugues Peccatte <hugues.peccatte@gmail.com>

MauricioFauth approved these changes May 18, 2019

View reviewed changes

Fix missing arguments

4b2980e

Signed-off-by: Hugues Peccatte <hugues.peccatte@gmail.com>

MauricioFauth merged commit 1a846ba into phpmyadmin:master May 19, 2019

MauricioFauth self-assigned this May 19, 2019

MauricioFauth added this to the 5.0.0 milestone May 19, 2019

Tithugues deleted the feature/DI branch May 19, 2019 15:56

MauricioFauth added a commit that referenced this pull request Jun 1, 2019

Add ChangeLog entry for #15266

01e59cf

External dependency injection system [ci skip] Signed-off-by: Maurício Meneghini Fauth <mauricio@fauth.dev>

This was referenced Jun 1, 2019

Inject dependencies #15271

Merged

Remove old DI container from various files #15279

Merged

MauricioFauth mentioned this pull request Jun 17, 2019

Remove old DI container #15335

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External dependency injection system #15266

External dependency injection system #15266

Tithugues commented May 11, 2019 •

edited

MauricioFauth commented May 11, 2019

Tithugues commented May 12, 2019

Tithugues commented May 12, 2019

MauricioFauth commented May 14, 2019

MauricioFauth commented May 14, 2019

Tithugues commented May 14, 2019

MauricioFauth commented May 14, 2019

Tithugues commented May 14, 2019

Tithugues commented May 14, 2019

MauricioFauth May 14, 2019

Tithugues May 17, 2019

codecov bot commented May 17, 2019 •

edited

williamdes left a comment

williamdes May 18, 2019

Tithugues May 18, 2019

MauricioFauth left a comment

MauricioFauth commented May 18, 2019

Tithugues commented May 18, 2019 •

edited

williamdes left a comment

Tithugues commented May 18, 2019

MauricioFauth commented May 18, 2019

Tithugues commented May 18, 2019

MauricioFauth commented May 19, 2019

Tithugues commented May 19, 2019

MauricioFauth commented May 19, 2019

External dependency injection system #15266

External dependency injection system #15266

Conversation

Tithugues commented May 11, 2019 • edited

Description

MauricioFauth commented May 11, 2019

Tithugues commented May 12, 2019

Tithugues commented May 12, 2019

MauricioFauth commented May 14, 2019

MauricioFauth commented May 14, 2019

Tithugues commented May 14, 2019

MauricioFauth commented May 14, 2019

Tithugues commented May 14, 2019

Tithugues commented May 14, 2019

MauricioFauth May 14, 2019

Choose a reason for hiding this comment

Tithugues May 17, 2019

Choose a reason for hiding this comment

codecov bot commented May 17, 2019 • edited

Codecov Report

williamdes left a comment

Choose a reason for hiding this comment

williamdes May 18, 2019

Choose a reason for hiding this comment

Tithugues May 18, 2019

Choose a reason for hiding this comment

MauricioFauth left a comment

Choose a reason for hiding this comment

MauricioFauth commented May 18, 2019

Tithugues commented May 18, 2019 • edited

williamdes left a comment

Choose a reason for hiding this comment

Tithugues commented May 18, 2019

MauricioFauth commented May 18, 2019

Tithugues commented May 18, 2019

MauricioFauth commented May 19, 2019

Tithugues commented May 19, 2019

MauricioFauth commented May 19, 2019

Tithugues commented May 11, 2019 •

edited

codecov bot commented May 17, 2019 •

edited

Tithugues commented May 18, 2019 •

edited