joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.77k stars 3.65k forks source link

API document rendered twice #39458

Closed SharkyKZ closed 1 year ago

SharkyKZ commented 1 year ago

What needs to be fixed

The document in the API application is rendered twice. First the document is rendered by the view but the result is not used:

Stack trace:
#0 C:\laragon\www\joomla-cms\libraries\src\MVC\View\JsonApiView.php(198): Joomla\CMS\Document\JsonapiDocument->render()
#1 C:\laragon\www\joomla-cms\api\components\com_content\src\View\Articles\JsonapiView.php(152): Joomla\CMS\MVC\View\JsonApiView->displayList()
#2 C:\laragon\www\joomla-cms\libraries\src\MVC\Controller\ApiController.php(255): Joomla\Component\Content\Api\View\Articles\JsonapiView->displayList()
#3 C:\laragon\www\joomla-cms\api\components\com_content\src\Controller\ArticlesController.php(86): Joomla\CMS\MVC\Controller\ApiController->displayList()
#4 C:\laragon\www\joomla-cms\libraries\src\MVC\Controller\BaseController.php(672): Joomla\Component\Content\Api\Controller\ArticlesController->displayList()
#5 C:\laragon\www\joomla-cms\libraries\src\Dispatcher\ApiDispatcher.php(61): Joomla\CMS\MVC\Controller\BaseController->execute('displaylist')
#6 C:\laragon\www\joomla-cms\libraries\src\Component\ComponentHelper.php(355): Joomla\CMS\Dispatcher\ApiDispatcher->dispatch()
#7 C:\laragon\www\joomla-cms\libraries\src\Application\ApiApplication.php(405): Joomla\CMS\Component\ComponentHelper::renderComponent('com_content')
#8 C:\laragon\www\joomla-cms\libraries\src\Application\ApiApplication.php(113): Joomla\CMS\Application\ApiApplication->dispatch()
#9 C:\laragon\www\joomla-cms\libraries\src\Application\CMSApplication.php(294): Joomla\CMS\Application\ApiApplication->doExecute()
#10 C:\laragon\www\joomla-cms\api\includes\app.php(53): Joomla\CMS\Application\CMSApplication->execute()
#11 C:\laragon\www\joomla-cms\api\index.php(31): require_once('C:\\laragon\\www\\...')
#12 {main}

Later the document is rendered in Joomla\CMS\Application\ApiApplication::render() when it's called inside the Joomla\CMS\Application\CMSApplication::execute():

Stack trace:
#0 C:\laragon\www\joomla-cms\libraries\src\Application\ApiApplication.php(150): Joomla\CMS\Document\JsonapiDocument->render(false)
#1 C:\laragon\www\joomla-cms\libraries\src\Application\CMSApplication.php(299): Joomla\CMS\Application\ApiApplication->render()
#2 C:\laragon\www\joomla-cms\api\includes\app.php(53): Joomla\CMS\Application\CMSApplication->execute()
#3 C:\laragon\www\joomla-cms\api\index.php(31): require_once('C:\\laragon\\www\\...')
#4 {main}

Why this should be fixed

Just.

How would you fix it

Remove one instance of render call. Preferably the one in the application.

Side Effects expected

Yes.

wilsonge commented 1 year ago

The view one should be removed so the API acts the same way as the web view controller and system plugins can do any manipulation of the document. I know there aren’t any system plug-in calls in the application at the moment but the PR has been pending for years :) https://github.com/joomla/joomla-cms/pull/34151/files

zero-24 commented 1 year ago

Since october Allon is awaiting your feedback within that PR ;)

wilsonge commented 1 year ago

He wants to remove the system plugins from it. Which is specifically the important part for this issue :)

SharkyKZ commented 1 year ago

The view one should be removed so the API acts the same way as the web view controller and system plugins can do any manipulation of the document. I know there aren’t any system plug-in calls in the application at the moment but the PR has been pending for years :) https://github.com/joomla/joomla-cms/pull/34151/files

That would be another step back. Going forward, the CMS should be modeled more like the framework where things actually make some sense. Even if the document concept remains, its creation should be pushed up into the controller. The application has no clue what document is needed or if it's needed at all. It's not needed at all on redirect pages, for example, and only the controller is aware of such context.

As for your PR, it can be closed as it makes no sense. It's clearly modeled after the fundamentally broken offline handling in the site application. Why would you prevent rendering the page but still allow users to make CRUD operations? Likewise, in site application users without offline login permissions can actually login and perform these operations by making direct POST requests. And the offline login screen is shown only on HTML pages. Other document types are not affected at all.

Ultimately, offline check needs to be performed early on, before the dispatch step. Near the MFA and password reset handling.

wilsonge commented 1 year ago

That would be another step back. Going forward, the CMS should be modeled more like the framework where things actually make some sense. Even if the document concept remains, its creation should be pushed up into the controller. The application has no clue what document is needed or if it's needed at all. It's not needed at all on redirect pages, for example, and only the controller is aware of such context.

No that's never going to be the case because both plugins and modules can e.g. add scripts to the DOM which need a document. The fact is there's more than just controllers in the Joomla Ecosystem - so it's the applications job to have a document that is format aware (e.g. feed views differentiated from html views) or json api from json ld formats. I accept it's not needed on every page - but frankly your at the controller level before you realise that - but a bunch of plugins have already run and potentially added scripts to the page (e.g. articles anywhere etc). Doing anything like that is incredibly shortsighted for the extension ecosystem.

wilsonge commented 1 year ago

PR https://github.com/joomla/joomla-cms/pull/39466

SharkyKZ commented 1 year ago

but a bunch of plugins have already run and potentially added scripts to the page

Something like this should only happen in poorly written plugins that create the document before the application does it, i.e. by calling Joomla\CMS\Factory::getDocument() during onAfterRoute or earlier event. These are already causing issues in J4 (and actually have been causing issues in J3, if not before that) anyways, but the method is deprecated so this shouldn't be a problem in the future. The earliest events where plugins can safely access the document are the events dispatched by the component itself anyways.

Modules are rendered by the document itself, so nothing changes for them.

so it's the applications job to have a document that is format aware

It's how it is now but it doesn't have to be. In the long term Joomla\CMS\Application\CMSWebApplicationInterface::getDocument() should be removed. Document class is very much like Joomla's equivalent of a renderer. It should exist only during the lifetime of a component. Plugins could still hook into it before it's rendered and during transition period the application's document object could be seeded.