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

[ticket/12775] Replace functions_container with a container_builder class. #2671

Merged
merged 34 commits into from Jul 11, 2014

Conversation

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jun 27, 2014

You can autoload the container factory and I think you should. I would also like to have the config file in its own class.

@Nicofuma Nicofuma changed the title [WIP][ticket/12775] Add a conter_factory class and remove functions_container [WIP][ticket/12775] Add a container_factory class and remove functions_container Jun 27, 2014
*
* @var ContainerInterface
*/
protected $container = null;

This comment has been minimized.

Copy link
@bantu

bantu Jun 27, 2014

Member

The null assignments are pointless.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 27, 2014

Author Member

for $container I agree, but for the others it make sense to me to explicitly assign them to null.

@Nicofuma

This comment has been minimized.

Copy link
Member Author

Nicofuma commented Jun 27, 2014

So you want to construct the auto_loader before checking if phpBB is installed?

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jun 27, 2014

Currently the class loader is already present when the container is built. You only have to load it because you're also handling config file, which I am not sure is a good idea as per my previous comment.

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jun 27, 2014

You will have to either load the config file class manally or functions.php (as per my PR).

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jun 27, 2014

The main point is that the config file has nothing to do with the container factory and thus should be a class of its own.

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jun 27, 2014

@Nicofuma

This comment has been minimized.

Copy link
Member Author

Nicofuma commented Jun 27, 2014

Updated with a config.php class
https://tracker.phpbb.com/browse/PHPBB3-12420 should be fixed
and for https://tracker.phpbb.com/browse/PHPBB3-12774 i don't know

@Nicofuma

This comment has been minimized.

Copy link
Member Author

Nicofuma commented Jun 27, 2014

and please, don't close your branch, your work is very useful. =)

if (file_exists($this->phpbb_root_path . 'config.' . $this->php_ext))
{
$x7eeee37ce4d5f1ce4d968ed8fdd9bcbb = null;
$x7eeee37ce4d5f1ce4d968ed8fdd9bcbb = get_defined_vars();

This comment has been minimized.

Copy link
@bantu

bantu Jun 27, 2014

Member

We can use a property with a non-weird name for that now.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 27, 2014

Author Member

indeed

*
* @return bool|array Return the content of the config file or false if the file does not exists.
*/
public function load_config_file()

This comment has been minimized.

Copy link
@bantu

bantu Jun 27, 2014

Member

get_all() ?
get_array() ?

This comment has been minimized.

Copy link
@bantu

bantu Jun 27, 2014

Member

might want to add get() as well

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 27, 2014

Author Member

updated

if ($this->inject_config)
{
$this->config_data = $this->config_php_handler->load_config_file();

This comment has been minimized.

Copy link
@bantu

bantu Jun 27, 2014

Member

Should just pass the object around I think.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 27, 2014

Author Member

updated

$phpbb_class_loader->register();
$config_php_handler = new \phpbb\config_php($phpbb_root_path, $phpEx);
$config_file_data = $config_php_handler->load_config_file();

This comment has been minimized.

Copy link
@bantu

bantu Jun 27, 2014

Member

Get rid of the variable. Just extract($foo->bar())

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 27, 2014

Author Member

updated

$phpbb_class_loader_ext = new \phpbb\class_loader('\\', "{$phpbb_root_path}ext/", $phpEx);
$phpbb_class_loader_ext->register();
// Set up container
$phpbb_container = phpbb_create_default_container($phpbb_root_path, $phpEx);
$factory = new \phpbb\di\container_factory($config_php_handler, $phpbb_root_path, $phpEx);

This comment has been minimized.

Copy link
@bantu

bantu Jun 27, 2014

Member

$phpbb_container_factory

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 27, 2014

Author Member

updated

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jun 27, 2014

The data from the config file must be available before constants.php is included.

@Nicofuma

This comment has been minimized.

Copy link
Member Author

Nicofuma commented Jun 27, 2014

updated

@nickvergessen

This comment has been minimized.

Copy link
Contributor

nickvergessen commented Jun 27, 2014

FILE: /home/travis/build/phpbb/phpbb/phpBB/includes/functions_container.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | Missing required file doc comment. (phpbb.Commenting.FileComment)
$phpbb_class_loader = new \phpbb\class_loader('phpbb\\', "{$phpbb_root_path}phpbb/", $phpEx);
$phpbb_class_loader->register();
$phpbb_config_php_handler = new \phpbb\config_php($phpbb_root_path, $phpEx);

This comment has been minimized.

Copy link
@bantu

bantu Jun 27, 2014

Member

I think the full path should be used here for much easier unit testing. See https://github.com/phpbb/phpbb/pull/2667/files#diff-71c3ea9241836355283e14e5e3171e6dR26

This comment has been minimized.

Copy link
@bantu

bantu Jun 27, 2014

Member

Why not just $phpbb_config_php?

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 27, 2014

Author Member

the full path for what?

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 27, 2014

Author Member

ha, i added a setters for that

This comment has been minimized.

Copy link
@bantu
@Nicofuma

This comment has been minimized.

Copy link
Member Author

Nicofuma commented Jun 27, 2014

For the sniffer issue, it's because the file has to be removed but i kept it for now (until the PR is ready)

if (file_exists($this->config_file))
{
$defined_vars = null;
$defined_vars = get_defined_vars();

This comment has been minimized.

Copy link
@bantu

bantu Jun 27, 2014

Member

What happens if config file contains $defined_vars? I suggested to use a property instead of a variable. Sure, config file could also define $this, but that really is out of scope.

This comment has been minimized.

Copy link
@bantu

bantu Jun 27, 2014

Member

All in all not that big of a deal though.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 27, 2014

Author Member

updated

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jun 27, 2014

I think this is much cleaner than functions_container.

@Nicofuma

This comment has been minimized.

Copy link
Member Author

Nicofuma commented Jun 27, 2014

It is much cleaner, maybe, but it is also much testable. And I still have to write the tests.

@Nicofuma Nicofuma changed the title [WIP][ticket/12775] Add a container_factory class and remove functions_container [ticket/12775] Add a container_factory class and remove functions_container Jun 27, 2014
@Nicofuma

This comment has been minimized.

Copy link
Member Author

Nicofuma commented Jun 27, 2014

tests added.

*
* @return string Path for dumped container
*/
protected function get_container_filename()

This comment has been minimized.

Copy link
@bantu

bantu Jun 28, 2014

Member

This makes sure we create a new container on the filesystem for each possible phpbb_root_path. Is this still required with the new code?

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jun 28, 2014

I do not like the variable name $phpbb_config_php_handler as per https://github.com/phpbb/phpbb/pull/2671/files#r14315417. There is no "handler" in the class name.

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jun 28, 2014

We might even want to consider moving the setup of the class loader into startup.php, the composer autoloader is being loaded there as well.

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jun 28, 2014

We may have ended up implementing a "Builder" instead of a "Factory" and might want to rename the class.

@Nicofuma

This comment has been minimized.

Copy link
Member Author

Nicofuma commented Jun 28, 2014

$phpbb_config_php could be confused with $config, but yes it's not a very good name

Nicofuma added 27 commits Jun 27, 2014
…vices.yml

PHPBB3-12775
bantu added a commit to bantu/phpbb that referenced this pull request Jul 11, 2014
[ticket/12775] Replace functions_container with a container_builder class.

* Nicofuma/ticket/12775: (34 commits)
  [ticket/12775] Set dbal.conn.driver as synthetic during installation
  [ticket/12775] Add the definition of dbal.conn in fixtures/config/services.yml
  [ticket/12775] Inject the connection when created in the container
  [ticket/12775] Extract the vars later in install/install_update.php
  [ticket/12775] Rename config_php_handler to config_php_file container_builder
  [ticket/12775] Set defined_vars as a property of config_php_file
  [ticket/12775] Fix doc blocks in the container builder
  [ticket/12775] Remove useless includes of config.php
  [ticket/12775] Move phpbb_convert_30_dbms_to_31 into the config file class
  [ticket/12775] Fix comments
  [ticket/12775] Update doc blocks
  [ticket/12775] Fix container_builder
  [ticket/12775] Rename config_php to config_php_file
  [ticket/12775] Renamed to \phpbb\di\container_builder
  [ticket/12775] Remove the last include of functions_container
  [ticket/12775] Fix unit tests
  [ticket/12775] Add tests for \phpbb\config_php
  [ticket/12775] Add tests for the container factory
  [ticket/12775] Use a field instead of a local var in load_config_var()
  [ticket/12775] Update container and config in install/
  ...
@bantu bantu merged commit 58a52fe into phpbb:develop-ascraeus Jul 11, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.