Skip to content
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

[4.0] fix for XTD plugins broken in frontend #23539

Closed
wants to merge 2 commits into from

Conversation

astridx
Copy link
Contributor

@astridx astridx commented Jan 14, 2019

Pull Request for Issue # #23510 . (Second try :))

Summary of Changes

I deleted the !$prefix condition from two if statements in the libraries/src/MVC/Factory/MVCFactory.php class. I was looking really carefully. But I can not find a place where$prefix = 'site'is correct if strpos($config['base_path'], '/administrator/') is true = if $config['base_path'] is set and contains the word '/administrator/'.

The condition !$prefix prevents that $prefix = 'Administrator'; can be set. $prefix is set in the line

$prefix = $this->app->getName();

to $prefix = 'Site;.

With this patch, no error will appear any more.

Testing Instructions / Expected result / Actual result

See #23510

Documentation Changes Required

No

What I noticed

What still confuses me is the fact that not all menu items can be selected.

I seldom work with the front end editing. That's why I did not notice that a publisher in xtd-modal does not see all menu items, which he can click in the font end. But: That was the same with Joomla 3x. I just checked that. Or is there something missing here?

@astridx astridx changed the title fix for XTD plugins broken in frontend [4.0] fix for XTD plugins broken in frontend Jan 14, 2019
@joomdonation
Copy link
Contributor

I haven't had time to read and understand current MVC implementation but I think MVCFactory should not contains magic code like that. Maybe a better solution for this problem would be override createView and createModel method directly in DisplayController of com_content frontend, something like below code:

	protected function createModel($name, $prefix = '', $config = array())
	{
		if (!$prefix && $this->input->get('view') === 'articles' && $this->input->get('layout') === 'modal')
		{
			$prefix = 'Administrator';
		}

		return parent::createModel($name, $prefix, $config);
	}

	
	protected function createView($name, $prefix = '', $type = '', $config = array())
	{
		if (!$prefix && $this->input->get('view') === 'articles' && $this->input->get('layout') === 'modal')
		{
			$prefix = 'Administrator';
		}

		return parent::createView($name, $prefix, $type, $config);
	}

The same logic would be applied for view = article and layout = pagebreak

@astridx
Copy link
Contributor Author

astridx commented Jan 16, 2019

@joomdonation Thank you for your comment.

I looked at it again. I am far away from properly understanding the MVCFactory class. But I think that the functionality belongs here. The function is not only needed in com_content. Each component may use a backend view in the frontend. That is why I think it is general and it belongs to the parent class

But: This special case must be specified in the child class. If we agree that in this case the variable $config['base_path'] is set, it would run properly. That's how it was in Joomla 3. But changes in Joomla 4 have caused that the variable $prefix is set and that is why the line I changed in this PR did not work any more.

I think this is what @mbabker means here: #23530 (comment) with

which is a regression from 3.x where the components were smart enough to proxy frontend requests to the backend code

I see no reason why $prefix needs to be checked in the if statement I changed.

@joomdonation
Copy link
Contributor

There must be a reason for that check. I am unsure but from my quick look, with your change might break the case when we try to create a frontend model (in this case, we would pass 'Site' as the value for $prefix parameter)

Also, as I mentioned, we are having too much magic code to determine the $prefix base on base_path config value. That's why I think override createModel and createView method when it's needed would be better.

For this, we need someone with better experience to make decision.

@ghost ghost mentioned this pull request Mar 28, 2019
@ghost ghost added the J4 Issue label Apr 5, 2019
@ghost ghost removed the J4 Issue label Apr 13, 2019
@SharkyKZ
Copy link
Contributor

@laoneo can you take a look at this?

@SharkyKZ
Copy link
Contributor

It seems that the prefix must always be set in Joomla\CMS\MVC\Controller\BaseController::getView(). Otherwise Joomla\CMS\MVC\Controller\BaseController::$views ends up with prefix-less views, which causes conflicts.

@astridx
Copy link
Contributor Author

astridx commented Jun 30, 2019

Close because of other PR #25367

@astridx astridx closed this Jun 30, 2019
@ghost
Copy link

ghost commented Jun 30, 2019

Thanks for this PR @astridx

@astridx astridx deleted the 23510_secondtry branch August 9, 2020 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants