? Pending

User tests: Successful: Unsuccessful:

avatar korenevskiy
korenevskiy
17 Dec 2022

Pull Request for Issue # .

Summary of Changes

Correction of the lack of the possibility of using custom View in the content component with custom ItemMenu.

Testing Instructions

  1. Add Custom Model CustomModel.php in JROOT\components\com_content\src\Model\
  2. Add Custom View HtmlView.php in JROOT\components\com_content\src\View\Custom\
  3. Add Custom ItemMenu default.xml in JROOT\components\com_content\tmpl\custom\
  4. Add Custom ItemMenu default.php in JROOT\components\com_content\tmpl\custom\
  5. Enable error display mode.
  6. Enable switch mode "Search Engine Friendly URLs"(SEO) in "Yes" in "Global Configuration" panel.
  7. Create a new menu item in the main menu for a new custom item.
  8. Open the user's page using a new custom link.
  9. Make sure that the error call is displayed in the file on the user's page.

Actual result BEFORE applying this Pull Request

Adding a menu item of its own type to the Content component caused an error.
There was NO such problem in the old Joomla 3.

The problem is relevant only for Joomla 4

Expected result AFTER applying this Pull Request

As well as the new fix, in the future developers in this repository will be able to add a new menu item without having to search for an error. By the way, it took me a week of working time to find the error. The location of the error correction is very difficult to determine, although the idea of the problem is immediately clear.

Also, this fix may in the future create a new menu item for the mod_category_tags module #38752

.

Link to documentations

Please select:

  • [V] No documentation changes for docs.joomla.org needed
  • [V] No documentation changes for manual.joomla.org needed

The problem is that php rules cause an error for views that are not specified in the file
JROOT\components\com_content\src\Service\Router.php

That is, this file has hard-coded representations that can be used without causing any errors. There was no such behavior in the old Joomla. Each developer could add their own views. And also I have repeatedly read tutorials on how to add a new type of custom menu item. This is not possible with Joomla 4. I suggest fixing this problem by simply adding 3 lines of code.

By the way, this error problem occurs only when the switch mode is turned on "Search Engine Friendly URLs"(SEO) in "Yes" in "Global Configuration" panel.

Of course, this mode is always on for everyone. So this fix is necessary for everyone who wants to add a new type of menu item. It is necessary for the future to upgrade J4.

avatar joomla-cms-bot joomla-cms-bot - change - 17 Dec 2022
Category Libraries
avatar korenevskiy korenevskiy - open - 17 Dec 2022
avatar korenevskiy korenevskiy - change - 17 Dec 2022
Status New Pending
avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar korenevskiy korenevskiy - change - 17 Dec 2022
Labels Added: ?
avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar SharkyKZ
SharkyKZ - comment - 17 Dec 2022

You should not be adding random files in extension directories. Use the MVC factory to add new parts to the MVC stack. Likewise, add your custom views to the router manually.

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

@SharkyKZ

You should not be adding random files in extension directories.

I didn't understand which extension directory you are writing about? Write the path.

Use the MVC factory to add new parts to the MVC stack.

Give an example in which file you need to write \Joomla\CMS\MVC\Factory\MVCFactory::create View($name, $prefix);

Likewise, add your custom views to the router manually.

Which router do you suggest I add manually? If when updating J4, this router will be overwritten. If this router is overwritten, then all your advice is very bad.

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

if ($item->query['view'] === $query['view']) {

There is another option, instead of these fixes, to use another fix: add a check for the presence of a view to this condition.

if ($item->query['view'] === $query['view'] && isset($views[$query['view']])) { 
...
}

I think it will be a more correct fix!!!
@SharkyKZ what do you think?

avatar richard67
richard67 - comment - 17 Dec 2022

@SharkyKZ

You should not be adding random files in extension directories.

I didn't understand which extension directory you are writing about? Write the path.

@korenevskiy I think @SharkyKZ did not mean your code changes. He means the use case described in your testing instructions, adding the files for custom model, custom view and custom item menu to the folders of a core component, in this case com_content, path JROOT\components\com_content and subfolders below that. If that is just for testing, then ok, but if that is the real use case then it is wrong because no custom files should be added there.

Besides this we have a semantic versioning and a backward compatibility (b/c) policy which is the reason why in 4.2 we will only apply bug fixes but no new features. New features have to go into 4.3 and so PRs for that have to be made for the 4.3-dev branch. And when 4.3 is in beta phase (currently it is in alpha phase) then it will have feature freeze, so new features have to go into 4.4 then. This PR here is not a bug fix but wants to introduce new possibilities, so it is clearly a thing which has to be made for the 4.3-dev branch.

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

@richard67 Thank you very much. I was familiar with the policy of major and minor versions. But I really don't know how to switch this PR to a new branch.
I made a mistake with the choice of a branch for PR
My fix is really a new functionality for Joomla 4. But this functionality is not new to Joomla 3.

And I still don't understand what is the point of fixing the original files for testing. After all, I have already tested everything, and spent a week determining the cause, the structure of the code and other things. My PR is not 15 minutes of work, my PR is 4 full days of work searching for problems accompanied by a lot of testing of the original code.
And again, and based on the correction, it would be possible to understand that I have been doing this for several days.

In this PR, I would like to discuss about placing this fix in the constructor of the same class or where I fixed it. Or maybe we need to fix the file.

if ($item->query['view'] === $query['view']) {

to

if ($item->query['view'] === $query['view'] && isset($views[$query['view']])) { 
...
}

@SharkyKZ Probably just didn't understand the description of PR.

avatar richard67
richard67 - comment - 17 Dec 2022

@korenevskiy It seems that you did not understand my question about the use case. So let me try it again. In your testing instructions you wrote:

  1. Add Custom Model CustomModel.php in JROOT\components\com_content\src\Model\
  2. Add Custom View HtmlView.php in JROOT\components\com_content\src\View\Custom\
  3. Add Custom ItemMenu default.xml in JROOT\components\com_content\tmpl\custom\

My question was: Is this only a for testing? Or is this the real use case that people shall be able to add custom files to these folders?

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

My question was: Is this only a for testing? Or is this the real use case that people shall be able to add custom files to these folders?

Answer 2, is this the real use case that people shall be able to add custom files to these folders

avatar korenevskiy korenevskiy - change - 17 Dec 2022
The description was changed
avatar korenevskiy korenevskiy - edited - 17 Dec 2022
avatar richard67
richard67 - comment - 17 Dec 2022

My question was: Is this only a for testing? Or is this the real use case that people shall be able to add custom files to these folders?

Answer 2, is this the real use case that people shall be able to add custom files to these folders

Then it is wrong because that should not be possible.

avatar brianteeman
brianteeman - comment - 17 Dec 2022

Then it is wrong because that should not be possible.

Anything is possible but I agree that this should not be allowed

avatar brianteeman
brianteeman - comment - 17 Dec 2022

And also I have repeatedly read tutorials on how to add a new type of custom menu item. This is not possible with Joomla 4

Can you point to one of these tutorials - it might break the language barrier and help to explain what you are trying to do.

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

Then it is wrong because that should not be possible.

  • Don't limit people.
  • This idea is not for creating components for sale. And to add a menu item, for a specific customer. It's just functionality.
  • I also agree with you that this should not be used to create extensions.
  • This will allow you to add new types of menu items to the Joomla core in an easier way, as an example of a similar operation of the mod_category_tags module.

And now I will tell you about a specific task. Which I had.

There are 3 sections of exhibitions on the museum's website. Future exhibitions, past exhibitions, current exhibitions. In fact, all these exhibitions are located in the same directory. But each exhibition has a custom date field, the beginning and end of the presentation.

I have created 3 files in the specified directories in the header. These 3 files inherited from the FEATURED menu item. But by adding a date condition to the query of the BD query. As well as the sorting mode for this date field. As a result, the site visitor could only see the real exhibitions, the future, or the past. I did this many many years ago when Joomla 4 was not there at all.

How do you advise me to do such functionality for Joomla 4. When you just need to be able to take into account the date condition.

I inherited my CUSTOM model from the FEATURED model, I inherited my CUSTOM view from the FEATURED view. In the model, I added a Date to the Query BD condition. In the view I added the layout name from render FEATURED

But in the same way I will be able to add other types of restrictions for articles, going beyond the Joomla core. For example, I can create a section to display articles ONLY for the New Year holidays.

This way I can add small functions for standard types of menu items of the Content component.
From the very beginning of my acquaintance with Joomla, I have always admired Joomla, that it is so universal. And that, if necessary, you can make your own type of menu.

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

Can you point to one of these tutorials - it might break the language barrier and help to explain what you are trying to do.

This is an unofficial tutorial, in one blog of a Russian-speaking developer dedicated to Joomla. I've been reading this blog for J2 or J3 for a long time, I don't remember when it was, 7-10 years ago. It seems to me that this blogger is also actively involved in the localization of Joomla and the improvement of this repository. But I don't remember the name, I can only find the blog portal, I can't find the article again.

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

Let's discuss what it means to do this badly. This is bad from the point of view of extension distribution. But maybe it's good from the point of view to make your own changes for an individual case. We're not going to throw everything into one pile and say that everything is not good at all.

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

As a developer, I need to solve the task. If I can't solve it with a tool, then why do I need this tool.
The task of CMS is not to limit just like that, but to create limiting rules. For example, by limiting myself to the MVC hierarchy and the structure of the content component, I can solve my problem. But I solve it within the rules. After all, the ultimate task is for the developer to be able to solve his task, and the task of CMS restrictions should serve so that there is no porridge, so that there is protection.
Or am I wrong? Or tell me how I need to solve my problem?

avatar joomdonation
joomdonation - comment - 17 Dec 2022

@korenevskiy As I understand, you want to register the view dynamically to component router. That would only work if it is a simple view (no key, no parent like archive or featured views from com_content). What if the view requires a key and need a parent (like article view, see how it is registered here https://github.com/joomla/joomla-cms/blob/4.2-dev/components/com_content/src/Service/Router.php#L92-L94).

So I would say that your solution is incomplete and won't be accepted. For your need, maybe override the Router service would be right solution, see the last entry on this forum topic https://forum.joomla.org/viewtopic.php?p=3639924 , it might help.

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

So I would say that your solution is incomplete and won't be accepted. For your need, maybe override the Router service would be right solution, see the last entry on this forum topic https://forum.joomla.org/viewtopic.php?p=3639924 , it might help.

Dear citizen. I took that into account. In my PR, this does not cause problems. Since the corrections in PR add dynamic verification only after setting rigid representations. At the same time, the dynamic task of the representation is checked for the presence of an already hard-coded representation.

See here

        $view = $query['view'] ?? false;

        if ($view && empty($this->getViews()[$view])) {
            $this->registerView(new RouterViewConfiguration($view));
        }

@joomdonation Do you see in the condition a check for the presence of hard-coded representations with specified ID parameters and others?

empty($this->getViews()[$view])

There is no problem. What you have described, I have tested many times and checked the work considering what you wrote to me. I took this into account when adding this PR

avatar joomdonation
joomdonation - comment - 17 Dec 2022

As I said, your code will only work for simple views only. With a more complicated view (like category view or article view in com_com_content), that won't work. Please try to look at com_content router https://github.com/joomla/joomla-cms/blob/4.2-dev/components/com_content/src/Service/Router.php#L79-L106 to see the extra code we need to handle these more complicated views.

I don't know how to explain better to you. If you still think that your solution is valid, @Hackwar can take a look and answer you further.

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

As I said, your code will only work for simple views only. With a more complicated view (like category view or article view in com_com_content), that won't work. Please try to look at com_content router https://github.com/joomla/joomla-cms/blob/4.2-dev/components/com_content/src/Service/Router.php#L79-L106 to see the extra code we need to handle these more complicated views.

I don't know how to explain better to you. If you still think that your solution is valid, @Hackwar can take a look and answer you further.

I already said that I checked. I have studied this class and watched it many times. I see how configurations for the view are set in this class. I took that into account. In the condition of my correction, there is a check for the presence of already existing view configurations. You should understand that my check takes place after the configurations are set in the file you specified. And I do a check in my code for the presence of this configuration, after executing the code of the file you specified.

So, my code is completely complete. And takes into account complex configurations of views.

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

@joomdonation
I understand what your problem is. Do you not know what this code means?

$this->getViews()

Do you think this function returns a list of views? You're wrong. This function is for something else entirely. This function returns a list of configuration (RouterViewConfiguration[]) for views.
You don't know this code, and that's why you think I'm wrong. But I remind you that it is you who are mistaken in the function $this->getViews()
By the way, I want to remind other participants of this PR, so that no one here will confuse this function.
@richard67 , @SharkyKZ , @brianteeman , @korenevskiy

avatar joomdonation
joomdonation - comment - 17 Dec 2022

I only see this code from your PR

$this->registerView(new RouterViewConfiguration($view));

Where do you register the key and register the parent if needed like here this example https://github.com/joomla/joomla-cms/blob/4.2-dev/components/com_content/src/Service/Router.php#L93? when it is needed

Where do you register it's parent and tell the system that it is a nested view like in this example https://github.com/joomla/joomla-cms/blob/4.2-dev/components/com_content/src/Service/Router.php#L90 when it is needed?

I don't see how these sample cases could be handled with your added code. I don't understand the routing code good enough but that's what's I see by reading your code quickly.

If you think that I'm wrong, that's fine. I will leave this to someone else to discuss with you further.

avatar SharkyKZ
SharkyKZ - comment - 17 Dec 2022

@joomdonation You are not wrong. This PR is wrong and should have been closed already.

avatar joomdonation
joomdonation - comment - 17 Dec 2022

Thanks @SharkyKZ

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

See here

// (heir) JROOT\components\com_content\src\Service\Router.php
class Router extends RouterView
{
    public function __construct(SiteApplication $app, AbstractMenu $menu, CategoryFactoryInterface $categoryFactory, DatabaseInterface $db)
    {
        $this->categoryFactory = $categoryFactory;
        $this->db              = $db;

        $params = ComponentHelper::getParams('com_content');
        $this->noIDs = (bool) $params->get('sef_ids');
        $categories = new RouterViewConfiguration('categories');
        $categories->setKey('id');
        $this->registerView($categories);
        $category = new RouterViewConfiguration('category');
        $category->setKey('id')->setParent($categories, 'catid')->setNestable()->addLayout('blog');
        $this->registerView($category);
        $article = new RouterViewConfiguration('article');
        $article->setKey('id')->setParent($category, 'catid');
    }
}
// (Parent)  JROOT\libraries\src\Component\Router\RouterView.php
abstract class RouterView extends RouterBase
{
    public function registerView(RouterViewConfiguration $view)
    {
        $this->views[$view->name] = $view;
    }
    public function getViews(): // return array RouterViewConfiguration[]
    {
        return $this->views;
    }
    public function build(&$query) // My Fix code
    {
        $view = $query['view'] ?? false;

        // Use the processing of a custom view added by the user.
        if ($view && empty($this->getViews()[$view])) {
            $this->registerView(new RouterViewConfiguration($view));
        }
    }
}

Look into this code, you can see how one file inherits another. You can see how to get the complex view configurations specified in the file via the getView() functions

Here's the proof. What is my correction not complete?

avatar joomdonation
joomdonation - comment - 17 Dec 2022

Sorry, I don't know how to explain better. Therefore, I give up and won't comment further.

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

@joomdonation

Please explain. Are you saying that my correction is incomplete? or do you claim that it will cause an error?
Mine first does a check for the presence of the specified configurations. If they are set, then nothing will be added.
We should not silently think something to ourselves. Will you explain to me why my code is not complete?

You can see how in the file constructor JROOT\components\com_content\src\Service\Router.php view configurations are set. I want to note that this happens in the constructor. This means that the call to the Bild function of this object already occurs after assigning all configurations for each representation of this component. In the Bild function, I check for the configurations of those that are already set in the constructor. And if there is no configuration, I create it. I will note that I do this not instead of those given configurations, but sequentially, and only later in the hierarchy. I.e. I will not replace the code that you specified to me in the file, but I do an additional check to the code that is executed in the file you specified.

That is, any reference to the Bild() function implies that this function will already be executed only after all the specified configurations of views.

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

Where is the error? What complex configuration will fail?
@SharkyKZ

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

What happened? Please write what is the problem?

I only see this code from your PR

$this->registerView(new RouterViewConfiguration($view));

@joomdonation
But you specified this code, and there is a condition before this code, why don't you take the condition into account?

avatar korenevskiy
korenevskiy - comment - 17 Dec 2022

@SharkyKZ Please tell what is the error of this PR?

avatar Hackwar
Hackwar - comment - 18 Dec 2022

This PR should NOT be merged. If you want to have additional views in com_content, you can add that with your own plugin and by extending the router somewhere on boot of Joomla, but simply adding all unknown views is definitely wrong and NOT something that we would ever want to have in the Joomla core. My first concern would be, that any typos in a URL would be hidden by this and we would instead get weird bug reports in our issue tracker. But there are more issues with this.

I'm sorry, but because of this, I'm going to close this one.

avatar Hackwar Hackwar - close - 18 Dec 2022
avatar Hackwar Hackwar - change - 18 Dec 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-12-18 15:46:01
Closed_By Hackwar

Add a Comment

Login with GitHub to post a comment