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

[#32146] com_ajax: improvement, use controllers, better error handling #2107

Closed
wants to merge 20 commits into from

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Oct 2, 2013

can be a bit late, but but...

this pull request change the com_ajax to work in Joomla! way, using new MVC

and work like described http://docs.joomla.org/Xml-rpc_changes_in_Joomla!_1.6
here each format have a own controller and each "element" (plugin, module) have own controller.
It allow more simple extend this component in future, if will be need

call link changed to:
for plugins:
index.php?option=com_ajax&task=plugin.call&name=foo&format=json
index.php?option=com_ajax&task=plugin.call&name=foo&format=raw

for modules:
index.php?option=com_ajax&task=module.call&name=foo&format=json
index.php?option=com_ajax&task=module.call&name=foo&format=raw

Another formats we can add in future, by request.
Other changes:

  • response format from com_ajax: Changes response format #1960 by @piotr-cz
  • format parameter now is required
  • own Exception handler that should fix previous problem with handling errors when "ajax" call,
  • own function for check "whether module available"
  • added content and system plugin groups

links:

@piotr-cz
Copy link
Contributor

piotr-cz commented Oct 2, 2013

What are reasons behind own AjaxModuleHelper instead of using native JModuleHelper and AjaxError?

Overall this feels much more right than current implementation

@Fedik
Copy link
Member Author

Fedik commented Oct 2, 2013

AjaxError for replace the default exception handler that Joomla! use libraries/cms/error/page.php ..
it need because, as you can see, it allow more simple handle the errors, and because default handler will render a HTML page, that no need in our case.

AjaxModuleHelper just small helper for prevent code duplication in the controllers.
Default JModuleHelper load the full modules list that enabled for a current page, that not very optimal, because for us just need to check the one module, and I not see big sense that the module should be exactly "for this page" or for "this language" ...
I think for us enough check whether module enabled and user have the access.

@wilsonge
Copy link
Contributor

wilsonge commented Oct 2, 2013

I have to say I'm not a massive fan of this. In this specific case I think splitting it up is creating more hassle than it's worth. I'm of the opinion for modules and plugins pretty much all we should support is json replies - and maybe raw if we are in debug mode. This seems like creating files for the sake of conforming to standards.

I support moving to JResponseJson (as in piotr's PR) but I think that's as far as we need to go.

I'd say this component will get removed as soon as we move to webservices (J5??) anyhow

@piotr-cz
Copy link
Contributor

piotr-cz commented Oct 3, 2013

@Fedik thanks for answers, this helps.
I'm not entirely convinced for the same reason as @wilsonge, on the other code has logic pattern now.

@elinw
Copy link
Contributor

elinw commented Oct 4, 2013

If we are going to do controllers let's do them new style for example just calll or callplugin, callmodule with options for format (and maybe even with options for plugin or module). The new style is designed for services and makes a lot more sense. Also use singular folder names so things autoload correctly and we don't need to mess around with includes.

@Fedik
Copy link
Member Author

Fedik commented Oct 4, 2013

@elinw there already task=module.call and task=plugin.call , and parameters format=raw, format=json also there.
Or I think I not very understand suggestion about "callplugin, callmodule".
Can you explain a bit more? :)

I know not to much about planed web-services, so I thought that extending the current realization by adding just a new controller in future, will be a cool idea ;)

and think, right, if do it compatible with web-services, then the current "trick" can be removed with the less pain in future when web-services will be ready ... but I know nothing about the plans around web-services :)
and whether web-services will allow interaction with the modules?

@betweenbrain
Copy link
Contributor

I appreciate the work that went into this PR, the code is well written and has some nice innovations like AjaxError. In no way do I want to discourage future contributions, but I'm also not convinced that introducing controllers is beneficial. @wilsonge is right in that it is very unlikely that we'll need to introduce other formats in the future and may introduce more maintenance than benefit.

@Fedik
Copy link
Member Author

Fedik commented Oct 4, 2013

I really see no problem with formats
check http://docs.joomla.org/Xml-rpc_changes_in_Joomla!_2.5
if need additional format, then just need add the new controller , and no need any changes in existing controller,

example for xml just need add module.xml.php where in the call method you can render xml, or use view for it

@piotr-cz
Copy link
Contributor

piotr-cz commented Oct 4, 2013

@Fedik Yes, you can do it in your own installation, but what about if you'd like to use formats like image or xml in a public extension?

@Fedik
Copy link
Member Author

Fedik commented Oct 4, 2013

answer is:

Another formats we can add in future, by request.

it can be added without big problem if it will be a really need ;)

but combination plugin + format=image can be problem (and maybe for other formats too), because the dispatcher can return multiple values ... but it a question how to call the plugin(s)

@Fedik
Copy link
Member Author

Fedik commented Oct 5, 2013

ok, added xml and image formats.
right, looks a bit "over coded" :)

@betweenbrain
Copy link
Contributor

Would it be safe to say that we can close this issue as there isn't consensus to the changes it introduces?

Conflicts:
	components/com_ajax/ajax.php
@Fedik
Copy link
Member Author

Fedik commented Dec 1, 2013

ok, changed to using new MVC,
and without change the request format, so can be tested with existing plugins/modules that already wrote for use com_ajax

thoughts? :)

Conflicts:
	components/com_ajax/ajax.php
@Fedik
Copy link
Member Author

Fedik commented Feb 8, 2014

new pull to staging #3071

@Fedik Fedik closed this Feb 8, 2014
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.

None yet

5 participants