[4.0] Move component categories helper to services #19667

laoneo · 2018-02-13T13:33:53Z

Pull Request for Issue #19580.

Summary of Changes

Introduces a boot functionality for components where they can register services in a container instead of providing helper classes which get loaded magically. Consumer of these services can then boot a component first to make sure, that the services are loaded and get an instance of ComponentInterface.

On a long term, all the extensions should be bootable and provide an interface to their environment.

Testing Instructions

The instructions below do test the old JCategories::getInstance() code and the new one by booting the component.

Create a second category for articles
Create an article custom field
Open an article on the front end

Expected result

All should work as before.

Actual result

All should work as before.

Documentation Changes Required

The new service based approach needs to be documented.

wilsonge · 2018-02-13T14:29:45Z

Honestly I think that we keep ContentCategories here - the purpose of this was to remove guessing of path location - whilst still having the class - not removing the class altogether. So can we add it back for now please? Even if we end up removing it in a future refactor I want to figure out how it looks in this format :)

wilsonge · 2018-02-13T14:30:07Z

libraries/src/Categories/Categories.php

-
-			include_once $path;
-		}
+		$categories = ComponentHelper::boot($parts[0])->getCategories(count($parts) > 1 ? $parts[1] : '');


wilsonge · 2018-02-13T14:31:45Z

libraries/src/Component/ComponentContainer.php

+	 *
+	 * @since  __DEPLOY_VERSION__
+	 */
+	public function getCategories($section = ''): Categories


You can't do this return typehint - it needs to be nullable (i.e. ?Categories) but it's only available in PHP 7.1 http://php.net/manual/en/migration71.new-features.php - so we'll have to go without one with a PHP 7.0 minimum

laoneo · 2018-02-13T14:36:56Z

So can we add it back for now please

You mean in a namespaced way or the old way?

mbabker · 2018-02-13T14:48:57Z

Well, a component with its own container doesn't have to follow a strict class naming convention (because the container will define where the service lives), whereas stuff using the legacy container would need to follow the old way. So, I think you could give it a properly namespaced class name since you're creating a container for com_content.

With that said though, since we support options setting in the constructor, we need to ensure the categories service isn't singleton in the container (i.e. it can't be a shared service). Otherwise you'll always get an object with the first object's options.

laoneo · 2018-02-13T15:23:31Z

Added a content specific service class in 1a119fd.

laoneo · 2018-02-14T07:22:32Z

I'v added an interface and trait to load the extensions and added it to the application.

laoneo · 2018-02-14T08:00:25Z

Removed the WIP. Would be good if @mbabker and @wilsonge can review it again. If all ok then I can move to the other services.

brianteeman · 2018-02-14T10:53:20Z

libraries/src/Categories/Categories.php

@@ -389,4 +362,43 @@ protected function _load($id)
 			$this->_nodes[$id] = null;
 		}
 	}
+
+	/**
+	 * Sets the options for this service. The given options do overwrite the


pedantic I know but the word "do" should be deleted

brianteeman · 2018-02-14T10:53:37Z

libraries/src/Categories/Categories.php

+			return;
+		}
+
+		// Merge the options to not loose the base config


lose not loose

wilsonge

There's one more thing to figure out here. Which is what does an extension do if it doesn't support categories. Does it's container getCategories method return null all the time. Or do we do some sort of NotSupported Exception type for these things rather than returning null?

wilsonge · 2018-02-14T13:54:26Z

libraries/src/Categories/Categories.php

+	 *
+	 * @since  __DEPLOY_VERSION__
+	 */
+	public function setOptions(array $options)


Can we move this back to the constructor please. I know this is a bit simpler to view but I risking people call setOptions and reprovisioning just seems vaguely boring. Let's do one risky thing at a time

I had to do it that way as in the ComponentContainer here , it is not possible to pass the options. If there is a way to set the options, then I can change it back to the constructor.

You make this an anonymous function like we do in all the service providers for the main DI Container https://github.com/joomla/joomla-cms/pull/19667/files#diff-a56b3bb3afcd610a7b1560ed05d8a146R17

wilsonge · 2018-02-14T13:57:55Z

libraries/src/Extension/ExtensionLoader.php

+ */
+trait ExtensionLoader
+{
+	use ContainerAwareTrait;


I thought best practice on this stuff was rather explicitly including a trait here was to add an abstract method into the trait and then ContainerAwareTrait into the Application thing.

laoneo · 2018-02-14T14:04:03Z

Or do we do some sort of NotSupported Exception type for these things rather than returning null?

At the moment it returns null. If we throw an exception, we always have to do a try/catch around it which would be for me overhead. I was even thinking about to return an empty array just to not always write another "if" check around the for loop.

mbabker · 2018-02-16T00:07:48Z

libraries/src/Extension/ExtensionLoader.php

+			return $container->get($serviceName);
+		}
+
+		$path = $extensionPath . '/services/services.php';


Personally I'd suggest a proper PHP class gets used over a procedural file receiving variables from a calling method's scope.

The idea behind this is to have something similar like the service declaration in Symfony. At the moment it is PHP only, but perhaps we add later support for service declaration trough YSON or YAML.
Another benefit is that we do not have to do some magic class compiling again. We can then even get rid of the dispatcher file and load the dispatcher trough the component container. That's my next plan when this one gets merged.

Symfony has a different style container than we do (they have a compiled container which is cached and built for production using code generators, we have a runtime container built through injected callables, which when they are Closures can't be serialized/cached in any sane way). So until we move our code to support compiled/cached containers like that, I don't think we need to consider possible support for other formats.

For now we should not focus too much on the service loading strategy. We can change that afterwards when I did the Dispatcher service pr. Is the rest ok?

wilsonge

Just add the dispatcher into this one, then add the service loading logic we discussed in Glip and I'll get it merged

laoneo · 2018-02-19T15:31:04Z

Changed the service registering to a class. I would like to do the dispatcher in it's own pr.

nibra

The ExtensionManager(Interface|Trait) looks more like a ComponentFactory to me...

nibra · 2018-02-19T23:06:51Z

administrator/components/com_content/services/provider.php

+ * @copyright   Copyright (C) 2005 - 2017 Open Source Matters, Inc. All rights reserved.
+ * @license     GNU General Public License version 2 or later; see LICENSE.txt
+ */
+


No namespace?

This is the only thing which get loaded without the namespace. In a first version it was just a plain php file which registered services in a container as you can see here.

nibra · 2018-02-19T23:08:27Z

components/com_content/Service/Category.php

@@ -6,15 +6,18 @@
 * @copyright   Copyright (C) 2005 - 2017 Open Source Matters, Inc. All rights reserved.
 * @license     GNU General Public License version 2 or later; see LICENSE.txt
 */
+namespace Joomla\Component\Content\Site\Service;


Services should be application independent. So why ...\Site\...?

It can even be removed, but for now George wants to have it in place, see the comment.

I'm fine with the class itself so far - I just don't see, why it is in a namespace that semantically restricts it to 'site'.

We don't have application agnostic component code. So we'd need a solution to that to get around that (mildly important in some ways) semantic detail.

nibra · 2018-02-19T23:26:17Z

libraries/src/Extension/ExtensionManagerTrait.php

+	 * @param   string  $extensionName  The extension name
+	 * @param   string  $extensionPath  The path of the extension
+	 *
+	 * @return  mixed  The extension


In self::bootComponent() you rely on the return value being a ComponentInterface. In general, mixed should never be used as return type.

I had an Interface ExtensionInterface before but removed it because it would be more of a marker interface without any function for now.

Nevertheless, the return type should be ComponentInterface as expected by bootComponent(), or you need to ensure the right type in bootComponent() - that's the price of mixed.

On a long term I have in mind to support multiple extension types. That's why mixed. I understand your argument.

If you want to keep mixed (which is ok for now), you need to check that you actually got a ComponentInterface complying object, like

public function bootComponent($component): ComponentInterface { // ... $extension = $this->loadExtension('component', $component, $path); if ($extension instanceof ComponenInterface) { return $extension; } throw new UnexpectedExtensionType("$component is not a component"); }

The return type declaration does it for us ;)

nibra

Questions are answered so far, just one thing left.

nibra · 2018-02-21T15:44:23Z

libraries/src/Extension/ExtensionManagerTrait.php

+	 * @param   string  $extensionName  The extension name
+	 * @param   string  $extensionPath  The path of the extension
+	 *
+	 * @return  mixed  The extension


If you want to keep mixed (which is ok for now), you need to check that you actually got a ComponentInterface complying object, like

public function bootComponent($component): ComponentInterface { // ... $extension = $this->loadExtension('component', $component, $path); if ($extension instanceof ComponenInterface) { return $extension; } throw new UnexpectedExtensionType("$component is not a component"); }

wilsonge · 2018-03-01T21:23:05Z

I think there's still quite a lot of tweaks to be made here - but this will do as a starting point :)

joomla-cms-bot added the PR-4.0-dev label Feb 13, 2018

Move component categories helper to services

5e73d6b

laoneo force-pushed the j4/boot branch from dd9b64b to 5e73d6b Compare February 13, 2018 13:35

CS

7d7bc87

wilsonge reviewed Feb 13, 2018

View reviewed changes

Fix unit tests

fdcf139

laoneo added 2 commits February 13, 2018 16:21

Use a class for the categories service instead of a service provider

1a119fd

Merge branch '4.0-dev' into j4/boot

efd56db

laoneo added 4 commits February 13, 2018 17:01

Make the sniffs happy

166c511

Move boot the extension helper

44c1140

Merge remote-tracking branch 'remotes/upstream/4.0-dev' into j4/boot

3282e31

Introduce extension loader interface

db25ff4

laoneo force-pushed the j4/boot branch from a057199 to db25ff4 Compare February 14, 2018 07:17

Revert not needed interface

5f126d3

laoneo force-pushed the j4/boot branch from 4636212 to 5f126d3 Compare February 14, 2018 07:20

laoneo changed the title ~~[4.0][WIP] Move component categories helper to services~~ [4.0] Move component categories helper to services Feb 14, 2018

CS

27f89f2

brianteeman reviewed Feb 14, 2018

View reviewed changes

libraries/src/Categories/Categories.php Outdated

return;

}

// Merge the options to not loose the base config

Copy link

Contributor

brianteeman Feb 14, 2018

Choose a reason for hiding this comment

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

lose not loose

Language fixes

ff0000f

wilsonge requested changes Feb 14, 2018

View reviewed changes

Make getConatiner abstract

80931ab

laoneo added 2 commits February 14, 2018 15:20

CS

9eadbb9

Distinguish between optional and required options

09e4574

laoneo force-pushed the j4/boot branch from 65fdb03 to 09e4574 Compare February 15, 2018 15:37

CS

ca128f7

mbabker reviewed Feb 16, 2018

View reviewed changes

laoneo added 2 commits February 16, 2018 08:51

Run isolated

91c907d

Merge branch '4.0-dev' into j4/boot

75b29ce

wilsonge requested changes Feb 18, 2018

View reviewed changes

laoneo added 7 commits February 19, 2018 15:48

Load the container trough a class

d5b6c8a

Load the container trough a class

82a0e73

Safe check

bf25d1c

Merge remote-tracking branch 'remotes/upstream/4.0-dev' into j4/boot2

8d8d41b

versions

16b7c71

Merge remote-tracking branch 'origin/j4/boot' into j4/boot

3a3328e

doc

5a9912c

nibra reviewed Feb 19, 2018

View reviewed changes

nibra requested changes Feb 21, 2018

View reviewed changes

laoneo and others added 5 commits February 22, 2018 19:51

Merge branch '4.0-dev' into j4/boot

161e90a

Merge branch '4.0-dev' into j4/boot

8ee0093

Merge branch '4.0-dev' into j4/boot

dfb3a61

Use ComponentInterface as a typehint for now

a2447d3

Use main function

084e0a1

wilsonge merged commit b0bbab1 into joomla:4.0-dev Mar 1, 2018

wilsonge deleted the j4/boot branch March 1, 2018 21:23

wilsonge added this to the Joomla 4.0 milestone Mar 1, 2018

laoneo mentioned this pull request Apr 3, 2018

[4.0] Add new Helper folder structure #17253

Closed

This was referenced May 23, 2018

[4.0] Module dispatcher #19834

Merged

[4.0] Plugin as services #20547

Merged

[4.0] Move component categories helper to services #19667

[4.0] Move component categories helper to services #19667

Conversation

laoneo commented Feb 13, 2018 • edited Loading

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

wilsonge commented Feb 13, 2018

Choose a reason for hiding this comment

wilsonge Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

laoneo commented Feb 13, 2018

mbabker commented Feb 13, 2018

laoneo commented Feb 13, 2018

laoneo commented Feb 14, 2018

laoneo commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laoneo commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonge left a comment • edited by mbabker Loading

Choose a reason for hiding this comment

laoneo commented Feb 19, 2018

nibra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laoneo Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

nibra Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

laoneo Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nibra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonge commented Mar 1, 2018

laoneo commented Feb 13, 2018 •

edited

Loading

wilsonge Feb 13, 2018 •

edited

Loading

wilsonge left a comment •

edited by mbabker

Loading

laoneo Feb 21, 2018 •

edited

Loading

nibra Feb 21, 2018 •

edited

Loading

laoneo Feb 21, 2018 •

edited

Loading