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

Add a text function for plugins #41081

Merged
merged 2 commits into from
Jun 29, 2023
Merged

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Jun 28, 2023

Summary of Changes

Similar text function for plugins as we do in #41079.

Testing Instructions

  • Open the back end
  • Go to the url /administrator/index.php?option=com_installer&view=install
  • Open the "Upload Package File" tab

Actual result BEFORE applying this Pull Request

All strings are translated.

Expected result AFTER applying this Pull Request

All strings are translated.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@sandewt
Copy link
Contributor

sandewt commented Jun 28, 2023

After installing the patch, the following notice messages appear:

Notice: Only variables should be passed by reference in C:\xampp\htdocs\bugtesting5\joomla\plugins\content\contact\services\provider.php on line 38

Notice: Only variables should be passed by reference in C:\xampp\htdocs\bugtesting5\joomla\plugins\content\emailcloak\services\provider.php on line 37

And so on.

Doesn't seem to be a problem of this patch, but an underlying problem.
Assigning to $container->get(DispatcherInterface::class) a separate variable, solves this problem.

@laoneo
Copy link
Member Author

laoneo commented Jun 28, 2023

Does the same issue happen also on the 4.4-dev branch. For me it gives the impression that you somehow screwed up Joomla. But the message is definitely not related to the current pr.

@sandewt
Copy link
Contributor

sandewt commented Jun 28, 2023

is definitely not related to the current pr

I have also encountered this problem before.

[EDIT: PHPStorm reports 'Only variables can be passed by reference']

@sandewt
Copy link
Contributor

sandewt commented Jun 28, 2023

But the message is definitely not related to the current pr.

Oops, I tested in Joomla 5.


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

@sandewt
Copy link
Contributor

sandewt commented Jun 28, 2023

I have tested this item ✅ successfully on c1ec401


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

1 similar comment
@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on c1ec401


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

@Quy
Copy link
Contributor

Quy commented Jun 28, 2023

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 28, 2023
@laoneo laoneo changed the title Add a text function for plugins [5.0] Add a text function for plugins Jun 29, 2023
@laoneo laoneo changed the title [5.0] Add a text function for plugins Add a text function for plugins Jun 29, 2023
@MacJoom MacJoom merged commit b9d646c into joomla:4.4-dev Jun 29, 2023
4 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 29, 2023
@laoneo laoneo deleted the plugins/text branch June 29, 2023 12:39
@laoneo laoneo added this to the Joomla! 4.4.0 milestone Jun 29, 2023
@HLeithner
Copy link
Member

I'm not so happy with this, a plugin is normally not a thing which is used for translation, I also would move the tmpl from the plugin to layouts which is more the character then a "full" template at least in my understanding.

@laoneo
Copy link
Member Author

laoneo commented Jun 29, 2023

This here helps to ease the transition. At the end of the day the template files should work the same, regardless if they are used in views, modules, plugins and layouts. So this here is needed to provide a similar context in all of them. There are legit use cases where template files are used in plugins. So this is fine for the current architecture. If one some point we harmonize the whole code base then it will be even easier when all the template files use the same functions.

@HLeithner
Copy link
Member

polluting the namespace/class for all plugins doesn't make sense anyway, better use a trait and the plugin that really needs this functionality should use it.

@brianteeman
Copy link
Contributor

Why is it only this plugin that is updated and not all the others

@HLeithner
Copy link
Member

Also this doesn't look like a proper "template engine" when you have to write the code your self:
https://github.com/Digital-Peak/joomla-cms/blob/c1ec40129dc59e9b21db1d64ab8ab7215fa66be6/plugins/installer/packageinstaller/src/Extension/PackageInstaller.php#L53-L55

and don't have at least a render() function

@sandewt
Copy link
Contributor

sandewt commented Jun 30, 2023

and don't have at least a render() function

Hey does such a render function look like?
Isn't this a proven technique in Joomla to render? See among others.

https://github.com/Digital-Peak/joomla-cms/blob/c1ec40129dc59e9b21db1d64ab8ab7215fa66be6/plugins/content/vote/src/Extension/Vote.php#L118-L121

@laoneo
Copy link
Member Author

laoneo commented Jun 30, 2023

Why is it only this plugin that is updated and not all the others

I will do this in 5.0 once it is upmerged, similar to #41079 and #41063.

Also this doesn't look like a proper "template engine" when you have to write the code your self

As I said, this pr is made for the current architecture. If you want to change it go ahead and make a pr. This here is fine for what we have now. Not saying it is the gold solution.

laoneo added a commit to Digital-Peak/joomla-cms that referenced this pull request Jun 30, 2023
@laoneo
Copy link
Member Author

laoneo commented Jun 30, 2023

@brianteeman here we go #41092.

@brianteeman
Copy link
Contributor

thanks - I guess I am/was confused why you change one here for 4.4 and all the others for 5.0

laoneo added a commit to Digital-Peak/joomla-cms that referenced this pull request Aug 17, 2023
laoneo added a commit that referenced this pull request Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants