Skip to content

[4.0] Helper factory#32633

Merged
rdeutz merged 4 commits intojoomla:4.0-devfrom
Digital-Peak:j4/helper/factory
Mar 25, 2021
Merged

[4.0] Helper factory#32633
rdeutz merged 4 commits intojoomla:4.0-devfrom
Digital-Peak:j4/helper/factory

Conversation

@laoneo
Copy link
Copy Markdown
Member

@laoneo laoneo commented Mar 10, 2021

Pull Request for pr #32291.

Summary of Changes

All over joomla are helpers manually loaded. Better to load them through a factory from the booted extension.

Testing Instructions

  • Open the dashboard.
  • Add the following code to the file administrator/modules/mod_quickicon/src/Helper/QuickIconHelper.php:
public static function getDataAjax()
{
	$data = ['test'];

	return $data;
}
  • Open the url /administrator/index.php?option=com_ajax&module=quickIcon&method=getData&format=json

Actual result BEFORE applying this Pull Request

  • Icons are loaded.
  • Ajax url shows {"success":true,"message":null,"messages":null,"data":["test"]}

Expected result AFTER applying this Pull Request

  • Icons are loaded.
  • Ajax url shows {"success":false,"message":"The file at mod_quickIcon\/helper.php does not exist.","messages":null,"data":null}

Documentation Changes Required

Should be documented on the same place where the new extension setup is documented.

@laoneo laoneo force-pushed the j4/helper/factory branch from 1af9414 to f8a5255 Compare March 10, 2021 15:49
@laoneo laoneo force-pushed the j4/helper/factory branch from f8a5255 to 8f2114f Compare March 10, 2021 15:49
@laoneo laoneo mentioned this pull request Mar 10, 2021
Closed
@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Mar 10, 2021

My eyes still hurt from editing ajax.php!

@thednp
Copy link
Copy Markdown
Contributor

thednp commented Mar 10, 2021

I've looked into changes and I think it looks solid.

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Mar 10, 2021

@SharkyKZ why downvote?

@laoneo laoneo force-pushed the j4/helper/factory branch from 02f4b48 to 9fdbace Compare March 11, 2021 06:52
@thednp
Copy link
Copy Markdown
Contributor

thednp commented Mar 11, 2021

Since my #30816 was pushed for 4.1, all of a sudden my interest in this dropped.

@thednp if we can bring #32633 somehow to fly, then the same pattern can be applied to templates.

I don't see that comin any time soon this year.

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Mar 11, 2021

I understand you...

@joomdonation joomdonation mentioned this pull request Mar 14, 2021
Closed
@joomdonation
Copy link
Copy Markdown
Contributor

joomdonation commented Mar 14, 2021

It is working as described. However, there are few things:

  • You do not validate and make sure the method exists before calling the method. For legacy module, we throws LogicException
$results = new LogicException(Text::sprintf('COM_AJAX_METHOD_NOT_EXISTS', $method . 'Ajax'), 404);
  • Look like you bypass the extension enabled check like in the legacy module:
if ($results === null && $moduleId && $table->load($moduleId) && $table->enabled)

I won't comment about the architecture change here because I'm not good at it and still trying to get myself familiar with architecture changes in J4.

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Mar 17, 2021

@joomdonation please comment inline in the code. Easier to understand.
@wilsonge do we need here also the release blocker label as the previous one had it?

joomdonation
joomdonation reviewed Mar 20, 2021
View reviewed changes
components/com_ajax/ajax.php Outdated
}
if ($moduleInstance instanceof \Joomla\CMS\Helper\HelperFactoryInterface && $helper = $moduleInstance->getHelper($class))
{
$results = $helper->{$method . 'Ajax'}();
Copy link
Copy Markdown
Contributor

@joomdonation joomdonation Mar 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, you call the method without check and make sure that the method exists before calling (like what we do with none namespace helper module https://github.com/joomla/joomla-cms/blob/4.0-dev/components/com_ajax/ajax.php#L99)

You also do not check and make sure the module is enabled, like what we are doing here https://github.com/joomla/joomla-cms/blob/4.0-dev/components/com_ajax/ajax.php#L64 for none namespace module

Copy link
Copy Markdown
Member Author

@laoneo laoneo Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you have another look, restored most of the ajax.php stuff and now the boot call is inside the checks.

@rdeutz rdeutz added this to the Joomla 4.0 milestone Mar 22, 2021
@dgrammatiko
Copy link
Copy Markdown
Contributor

dgrammatiko commented Mar 24, 2021

I have tested this item ✅ successfully on 50828d9

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32633.

1 similar comment
@jwaisner
Copy link
Copy Markdown
Member

jwaisner commented Mar 25, 2021

I have tested this item ✅ successfully on 50828d9

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32633.

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.0 milestone Mar 25, 2021
@jwaisner
Copy link
Copy Markdown
Member

jwaisner commented Mar 25, 2021

RTC

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32633.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 25, 2021
@rdeutz rdeutz merged commit 1f54b31 into joomla:4.0-dev Mar 25, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 25, 2021
@rdeutz rdeutz added this to the Joomla 4.0 milestone Mar 25, 2021
@laoneo laoneo deleted the j4/helper/factory branch April 1, 2021 07:13
@dgrammatiko dgrammatiko mentioned this pull request Apr 18, 2021
Merged
@dgrammatiko dgrammatiko mentioned this pull request Oct 9, 2021
Closed
@laoneo laoneo mentioned this pull request Mar 29, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@joomdonation joomdonation joomdonation left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Joomla 4.0

Development

Successfully merging this pull request may close these issues.

7 participants

@laoneo @thednp @joomdonation @dgrammatiko @jwaisner @rdeutz @joomla-cms-bot