[4.0] Set the cli as global app by laoneo · Pull Request #16540 · joomla/joomla-cms

laoneo · 2017-06-05T16:50:07Z

As we introduced the CMSApplicationInterface the cli apps should act as global applications. This is a followup of #16499 and #16516.

Additionally the apps from the controllers are loaded from the container.

~~DON'T Merge, please!~~

…onvert

laoneo · 2017-06-06T06:44:21Z

WIll reopen when ready.

…onvert # Conflicts: # libraries/src/CMS/Service/Provider/Session.php

wilsonge · 2017-06-14T16:22:56Z

libraries/src/CMS/Application/CliApplication.php

 	 *
-	 * @since   11.1
+	 * @since       11.1
+	 * @deprecated  5.0 Load the app trough the container


wilsonge · 2017-06-14T16:24:10Z

includes/app.php


 // Instantiate the application.
-$app = JFactory::getApplication('site');
+$app = JFactory::getContainer()->get('SiteApplication');


I think this and AdministratorApplication still need to run through JFactory::getApplication otherwise we're going to end up with the JApplicationCms::$instance being empty right?

This has to be reverted back otherwise none of the singletons will be populated. JFactory::$application and the JApplicationCms instances container.

Can we not set them manually like I did with the cli scripts?

You can set JFactory::$application manually, but without Reflection, you can't set JApplicationCms::$instances.

I will revert that change and do it then in a new pr.

wilsonge · 2017-06-14T16:24:55Z

build/helpTOC.php


 		// JSON encode the file and write it to JPATH_ADMINISTRATOR/help/en-GB/toc.json
-		file_put_contents(JPATH_ADMINISTRATOR . '/help/en-GB/toc.json', json_encode($toc));
+		file_put_contents(JPATH_ADMINISTRATOR . '/help/toc.json', json_encode($toc));


Why are we removing en-GB here?

As this folder doesn't exist, either we remove it or create it. But like it is now it crashes.

It's supposed to exist - https://github.com/joomla/joomla-cms/tree/staging/administrator/help/en-GB - probably something that needs to be worked on from the Mediawiki PR.

Looks like i accidently deleted it :/ oops. 98cef84#diff-41ac6b5cb48181a44ad59de73a6fc4dd

mbabker · 2017-06-14T16:41:21Z

libraries/src/CMS/Service/Provider/Session.php

 					$dispatcher = $container->get('Joomla\Event\DispatcherInterface');
-					$dispatcher->addListener('onAfterSessionStart', array(JFactory::getApplication(), 'afterSessionStart'));
+
+					if (JFactory::getApplication() instanceof CMSApplication)


Why? If anything this should be a method_exists() check. This needlessly restricts the session service provider.

mbabker · 2017-06-15T12:49:53Z

libraries/src/CMS/Router/SiteRouter.php

 	{
-		$this->app  = $app ?: CMSApplication::getInstance('site');
-		$this->menu = $menu ?: $this->app->getMenu();
+		$this->app  = $app ?: \JFactory::getApplication();


I know the typehinting here is for the base CMSApplication, but are we certain that there isn't anything buried in the internals of the site router and menu that could cause a problem if the admin application were used in place of the site application?

mbabker · 2017-06-15T12:52:44Z

libraries/src/CMS/Exception/ExceptionHandler.php

-				if (\JFactory::$document)
+				if ($isCli)
 				{
 					// We're probably in an CLI environment


The comment here is actually very misleading, I don't think it even applies anymore. The comment above the if statement is more accurate, so I think the change you made should be reverted.

The intent was to use the document format type if there is already a JDocument instance in JFactory, and if that isn't set to fall back on the format in the request (obviously this part is web specific).

mbabker · 2017-06-15T12:54:02Z

libraries/src/CMS/Exception/ExceptionHandler.php

+					echo $app->toString();

-				$app->close(0);
+					$app->close(0);


$app->close() should be called regardless of being a CLI or web app. Or it can be removed completely as we'll hit the return line below and whatever PHP does after calling the uncaught exceptions handler would be performed.

Then we need to add that function to the CMSApplicationInterface?

I'm starting to think we can get away with just removing $app->close() completely and just using the return statement a few lines down, regardless of context.

mbabker · 2017-06-15T12:54:55Z

libraries/src/CMS/Router/SiteRouter.php

-		$this->app  = $app ?: CMSApplication::getInstance('site');
-		$this->menu = $menu ?: $this->app->getMenu();
+		$this->app  = $app ?: \JFactory::getApplication();
+		$this->menu = $menu ?: $this->app->getContainer()->get(MenuFactoryInterface::class)->createMenu('site', ['app' => $this->app]);


Why this change? This just seems like adding complexity for the sake of it, unless you plan on removing getMenu() from the CMSApplication class that was just fine.

I'm working actually on the cli scripts. Running the indexer from the command line needs the router which needs the site menu.

The typehinting here is still for the web application class chain, not the interface yet. So it's still wouldn't support a CLI app here anyway.

The next issue is the app interface isn't container aware, so you can't rely on getContainer() being available either.

laoneo · 2017-06-15T13:30:40Z

I'v changed all the input from Michael. Can you guys please have a second look?

wilsonge · 2017-06-28T21:00:47Z

libraries/src/CMS/Exception/ExceptionHandler.php

 				}

-				$app->setBody($data);
+				if ($isCli)


This makes me wonder if setBody and getBody should be in the interface. Thoughts @mbabker

I wouldn't try finangling it to fit. CLI and web apps handle output differently (CLI with output objects, web with response objects, both different APIs). So this is the best option.

wilsonge · 2017-06-28T21:01:20Z

This looks largely good. I want Michael's thoughts on the setBody/getBody thing but once we have that and we've solved conflicts I think this is probably good to merge

…onvert # Conflicts: # libraries/src/CMS/Factory.php # libraries/src/CMS/Router/SiteRouter.php

laoneo added 7 commits June 4, 2017 12:59

Adapt cli scripts

60f2def

Make update script work

34e0e8f

Fixing the core to work with the interface

2426093

Merge remote-tracking branch 'remotes/upstream/4.0-dev' into j4/cli-c…

da62d1e

…onvert

Use the cli app as global application

b15341f

Revert debug code

4f30c3d

Revert debug code

1d764bf

joomla-cms-bot added the PR-4.0-dev label Jun 5, 2017

laoneo closed this Jun 6, 2017

laoneo added 7 commits June 6, 2017 08:45

Raw rednerer

41f6dc4

Merge remote-tracking branch 'remotes/upstream/4.0-dev' into j4/cli-c…

2337bec

…onvert # Conflicts: # libraries/src/CMS/Service/Provider/Session.php

Just throw the error when in CLI mode

768f192

Get the router directly

fa6ec9a

Use the isClient function

b39421a

Change to interface

4033c9e

Change cli scripts to use the container

f5713d9

laoneo reopened this Jun 14, 2017

laoneo added 12 commits June 14, 2017 17:28

Convert help toc cli app

f7bc18e

Deprecate getInstance

76e8e47

Check for correct class

8fdbfbd

Revert workaround

4ffd3dd

Revert

48f879a

Revert

988a783

Revert

1a4561a

Revert

409c27b

cs

a276b19

Load the CMS apps trough the container

7f0929a

Load the app from the factory

50115d6

Revert

1550389

wilsonge reviewed Jun 14, 2017

View reviewed changes

mbabker reviewed Jun 14, 2017

View reviewed changes

laoneo added 2 commits June 15, 2017 13:04

Spelling

8448201

Add toc

e7b980e

laoneo force-pushed the j4/cli-convert branch from 3031478 to e7b980e Compare June 15, 2017 11:15

laoneo added 6 commits June 15, 2017 13:16

Revert app load

7a4557b

Revert toc path

271c6d6

Load the whole framework

c7e93d4

Add cli error renderer

e03f3f3

Get the menu from the container

81c96af

nl

0d952f3

mbabker reviewed Jun 15, 2017

View reviewed changes

uc

4ff357a

mbabker reviewed Jun 15, 2017

View reviewed changes

laoneo added 3 commits June 15, 2017 15:10

meethod exists check

5a8df19

remove close

bf7f32f

Load the app from getInstance

9b6bafb

wilsonge reviewed Jun 28, 2017

View reviewed changes

laoneo added 2 commits June 29, 2017 07:28

Merge remote-tracking branch 'remotes/upstream/4.0-dev' into j4/cli-c…

fcf6d66

…onvert # Conflicts: # libraries/src/CMS/Factory.php # libraries/src/CMS/Router/SiteRouter.php

Revert

47ef3d8

wilsonge merged commit fdd2644 into joomla:4.0-dev Jun 29, 2017

wilsonge deleted the j4/cli-convert branch June 29, 2017 07:40

wilsonge added this to the Joomla 4.0 milestone Jun 29, 2017

laoneo mentioned this pull request Jun 30, 2017

[4.0] Sync app setup #16918

Closed

Uh oh!

Conversation

laoneo commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laoneo commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laoneo commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilsonge commented Jun 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

laoneo commented Jun 5, 2017 •

edited

Loading