-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[4.0] Introduce router service #20339
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
Conversation
| * @param DatabaseInterface $db The database object | ||
| */ | ||
| public function __construct($app = null, $menu = null) | ||
| public function __construct(CMSApplication $app, AbstractMenu $menu, Category $category, DatabaseInterface $db) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we don't need a factory - these are all defined classes - so we should just use the DIC's build method here rather than using a factory (https://github.com/joomla-framework/di/blob/master/src/Container.php#L182) - we only need a factory for when we have the magic configuration array's that the DIC doesn't understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type of $app can be changed to SiteApplication and Category to Categories, but for the AbstractMenumenu we do not have a resource in the container.
…r/service # Conflicts: # administrator/components/com_content/Extension/ContentComponent.php
|
Getting rid of the factory would also mean that we set a router in the component directly and do always return the same instance on |
|
yes but you wrap it in an anonymous function so it's still lazy loaded |
|
When you create the |
…r/service # Conflicts: # administrator/components/com_content/Extension/ContentComponent.php # components/com_content/Router/ContentRouter.php
|
I need to get this one merged to finish the service conversion of com_contact, com_newsfeeds and com_categories. |
|
TBH it probably only needs the menu, especially for building routes across
applications. Not staring at code but I don’t remember the application
being too heavily used in component routers.
On Thu, Aug 30, 2018 at 5:54 PM George Wilson ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In components/com_content/Router/RouterFactory.php
<#20339 (comment)>:
> + {
+ $this->category = $category;
+ $this->db = $db;
+ }
+
+ /**
+ * Creates a router.
+ *
+ * @param CMSApplicationInterface $application The application
+ * @param AbstractMenu $menu The menu object to work with
+ *
+ * @return RouterInterface
+ *
+ * @SInCE __DEPLOY_VERSION__
+ */
+ public function createRouter(CMSApplicationInterface $application, AbstractMenu $menu): RouterInterface
If we have a SiteApplication here (we are aborting if we don't) - why
can't we use it's getMenu method? Is there ever a time we need any other
instance?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20339 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAWfoQuvfLPVzDqbwQONfFo6NV9_vzVvks5uWG07gaJpZM4T52Bw>
.
--
- Michael Please pardon any errors, this message was sent from my iPhone.
|
|
Any more feedback? |
…r/service # Conflicts: # administrator/components/com_finder/Extension/FinderComponent.php # administrator/components/com_finder/services/provider.php
|
Thankyou! |
Pull Request for Issue #19580.
Summary of Changes
The component router is available through the booted component instance when available as services.
Testing Instructions
Expected result
SEF urls are generated without id's.
Actual result
SEF urls are generated without id's.