Skip to content

[4.0] Add getLanguage to application interface and cleanup console/cli apps#28506

Merged
wilsonge merged 2 commits intojoomla:4.0-devfrom
wilsonge:interfaces_cli
Mar 29, 2020
Merged

[4.0] Add getLanguage to application interface and cleanup console/cli apps#28506
wilsonge merged 2 commits intojoomla:4.0-devfrom
wilsonge:interfaces_cli

Conversation

@wilsonge
Copy link
Copy Markdown
Contributor

@wilsonge wilsonge commented Mar 29, 2020

Partial Pull Request for Issue #24587 .
Fixes #27007

This ones a bit long - i started by adding the interface and then realised how broken CliApplication was when actually implementing/testing it

Summary of Changes

  • Adds getLanguage to the CMSApplicationInterface
  • Adds these methods to the two cli applications which previously didn't have them
  • Adds missing getInput method to the CliApplication which was in the interface but missing
  • Fixes typo in triggerEvent function inCliApplication
  • Adds property messages used for enqueueMessage in the CliApplication but wasn't defined
  • Fix CliApplication calling wrong parent method
  • Various fixes to finder indexer plugin to make it work

Testing Instructions

Check CLI application - see commands below If you want you can access any web page too to ensure the interface in the web applications doesn't error too.

Commands to run:
php cli/joomla.php (you can also run as many of the commands from here as you'd like to prove this)
php cli/finder_indexer.php

Expected result

Working console apps when new method added to interface

Actual result

Finder Indexer Broken

Documentation Changes Required

Yup interface documentation

@richard67
Copy link
Copy Markdown
Member

I have tested this item ✅ successfully on 6b6000b

All commands of cli/joomla.php still work.
cli/finder_indexer.php still works.
Com_finder in general still works.
Backend and frontend still work.
I hope I haven't forgotten anything.


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

@wilsonge
Copy link
Copy Markdown
Contributor Author

Worth noting before this patch the com_finder cli was broken :) so that one is fixed with this patch

@richard67
Copy link
Copy Markdown
Member

Worth noting before this patch the com_finder cli was broken :) so that one is fixed with this patch

Any issue exists which we can close?

@richard67
Copy link
Copy Markdown
Member

P.S.: Can confirm it fixes broken cli/finder_indexer.php.

@richard67
Copy link
Copy Markdown
Member

@wilsonge I've found issue #27007 ... does that fit?

@wilsonge
Copy link
Copy Markdown
Contributor Author

That's the one - didn't know there was an issue :) but added bonus!

@richard67
Copy link
Copy Markdown
Member

@skurvish Please test if this PR here solves your issue #27007 , and if so, mark your test result at the issue tracker https://issues.joomla.org/tracker/joomla-cms/28506 by using the "Test this" button, selecting the appropriate result (Success I hope) and the submitting.

@skurvish
Copy link
Copy Markdown

Will do, busy on a few other things right now but given the isolation I should find time over the next few days.

@skurvish
Copy link
Copy Markdown

I am not able to test this as the commits for the PR do not seem to be in the 4.0-dev branch.

@richard67
Copy link
Copy Markdown
Member

@skurvish It is a pull request (PR) and so not merged yet into the 4.0-dev branch. It needs test before. You can find here a full install download package which includes the patch of this PR: https://ci.joomla.org:444/artifacts/joomla/joomla-cms/4.0-dev/28506/downloads/30756/Joomla_4.0.0-beta1-dev+pr.28506-Development-Full_Package.zip. Or you have a 4.0-dev installation and apply the patch to that, e.g. using the Patchtester component.

@skurvish
Copy link
Copy Markdown

I can confirm that both the joomla and finder cli apps work. I would like to test my own cli if I can before I agree all is well. Quick question, will this be ported to J3.9 or will this be a J4 only implementation?


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

@richard67
Copy link
Copy Markdown
Member

@skurvish Could you mark your test result at the issue tracker https://issues.joomla.org/tracker/joomla-cms/28506 by using the "Test this" button, selecting the appropriate result (Success I hope) and then submitting? Thanks in advance.

As I can see this is a fix for J4. No idea if it can and will be backported to J3.

@wilsonge
Copy link
Copy Markdown
Contributor Author

This doesn’t need to get backported to 3.9 - these changes are only required due to work in the 4.0 branch. I will be documenting what changes you need to make to have things work on 3.9 and 4.0 for cli scripts though :)

@skurvish
Copy link
Copy Markdown

That would be awesome @wilsonge.


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

@wilsonge
Copy link
Copy Markdown
Contributor Author

There are 4 main things:

  1. if you are interfacing with core extensions you'll need to load the namespacemapper in your cli script https://github.com/joomla/joomla-cms/pull/28506/files#diff-64483624db166a09dbe03732dc7f7893R147 - this ones a bit trickier - I haven't experimented yet - but instead of the trait you may just need to include these 3 lines inside the doExecute method and inside a if (version_compare(JVERSION, '4.0', 'ge'))

  2. You'll need to add a getName function to your CLI script https://github.com/joomla/joomla-cms/pull/28506/files#diff-64483624db166a09dbe03732dc7f7893R339 that will return a unique string to identify your application (this will work fine in all Joomla versions so you can do this immediately from 3.9 without issues)

  3. You'll need to boot your application slightly differently by using the Dependency Injection Container rather than using JApplicationCli::getInstance like we do here https://github.com/joomla/joomla-cms/pull/28506/files#diff-64483624db166a09dbe03732dc7f7893L473-L491 (you should be able to do an if/else with a JVersion condition here)

  4. The usual stuff around the use of deprecated code in 3 that we have removed in 4 (obviously this applies just as much to CLI scripts as everything else)

@skurvish
Copy link
Copy Markdown

Great, thanks. I am out tomorrow but can see if I can integrate and test these changes Tuesday/Wednesday and report back. If I run into any issues where should I report them, here somewhere else?


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

@wilsonge
Copy link
Copy Markdown
Contributor Author

Yeah just post here. I'm going to merge this now as I need this interface change to unblock several other things. If it becomes complex I may get you to open a new issue if needs arise. But here will do as a starting point :) Thank you in advance for any feedback will be really valuable for me to help document migration paths!

@wilsonge wilsonge merged commit 18a7814 into joomla:4.0-dev Mar 29, 2020
@wilsonge wilsonge deleted the interfaces_cli branch March 29, 2020 23:23
@wilsonge wilsonge added this to the Joomla 4.0 milestone Mar 29, 2020
@skurvish
Copy link
Copy Markdown

skurvish commented Apr 2, 2020

I was not able to get the cli app to work without including the joomla configuration in the container. In order to accomplish this I needed to include_once( configuration.php), then add as the second param in the return statement: new Registry(new \JConfig).

My app does rely on the the config file and to use Factory::getConfig() in my app required this. I suppose I could have done the same thing in my app but this made more sense to inject it into the ApplicationCli container.

@skurvish
Copy link
Copy Markdown

skurvish commented Apr 2, 2020

@wilsonge I am also trying to get the ApplicationWeb interface to work. When I create the container then run the getContainer, the app object returned does not have a dispatcher attached to it (i.e. it is null in the object). Did you look at the web interface and might it require similar changes you made to the cli interface?

@skurvish
Copy link
Copy Markdown

skurvish commented Apr 4, 2020

@wilsonge Hey George, I solved my problems. Turns out in J4 I need to use the CMSApplication class instead of WebApplication. I must also set the dispatcher and session in order to load plugins.

@wilsonge
Copy link
Copy Markdown
Contributor Author

wilsonge commented Apr 8, 2020

I was not able to get the cli app to work without including the joomla configuration in the container. In order to accomplish this I needed to include_once( configuration.php), then add as the second param in the return statement: new Registry(new \JConfig).

If you boot your app with the container like here https://github.com/joomla/joomla-cms/blob/4.0-dev/cli/finder_indexer.php#L490-L514 that shouldn't be an issue.

I am also trying to get the ApplicationWeb interface to work. When I create the container then run the getContainer, the app object returned does not have a dispatcher attached to it (i.e. it is null in the object). Did you look at the web interface and might it require similar changes you made to the cli interface?

You'll need to set the dispatcher into the app yourself. See how we boot up the web application classes ourselves in the container https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Service/Provider/Application.php#L58-L69

There's nothing missing as far as I know. However don't have any custom web apps to hand to test for you either to give a better idea. Can you give some sample code if the above doesn't help

@skurvish
Copy link
Copy Markdown

skurvish commented Apr 9, 2020

I attempted to use the container setup exactly like the finder app but the configuration was not available in my app and numerous errors occurred. Inserting the config as stated solved the issue.

Your examples for the web app show it using the SiteApplication and AdministratorApplication classes. I attempted to use the WebApplication class which is what I used in J3.x but that didn't work at all. I changed to using the CMSApplication class and that at least instantiated OK but doing the $container->get(DispatcherInterface::class) returned null so I created a dispatcher using new \Joomla\Event\Dispatcher and set it using the setDispatcher method.

I am not sure any of this is right but it is working. I would be happy to keep working on it to try new things to sort it out properly if you would like. I guess I would rather get it correct and working than just working and perhaps not in the preffered/correct manner.

@skurvish
Copy link
Copy Markdown

@wilsonge did you want me to try anything else?

@infograf768 infograf768 changed the title Add getLanguage to application interface and cleanup console/cli apps [4.0] Add getLanguage to application interface and cleanup console/cli apps Apr 20, 2020
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