Skip to content

Comments

[4.0] Get menu directly in com_tags menu route helper#30039

Merged
richard67 merged 3 commits intojoomla:4.0-devfrom
SharkyKZ:j4/fix/cli-finder-indexer-tags
Jul 20, 2020
Merged

[4.0] Get menu directly in com_tags menu route helper#30039
richard67 merged 3 commits intojoomla:4.0-devfrom
SharkyKZ:j4/fix/cli-finder-indexer-tags

Conversation

@SharkyKZ
Copy link
Contributor

@SharkyKZ SharkyKZ commented Jul 7, 2020

Summary of Changes

Fixes error when running Finder CLI.

Testing Instructions

Install Testing Sample Data or create some articles with tags.
Run php cli/finder_indexer.php

Actual result BEFORE applying this Pull Request

Symfony\Component\ErrorHandler\Error\UndefinedMethodError^ {#688
  #message: "Attempted to call an undefined method named "getMenu" of class "FinderCli"."
  #code: 0
  #file: "C:\wamp\www\joomla-cms\components\com_tags\src\Helper\RouteHelper.php"
  #line: 157
  trace: {
    C:\wamp\www\joomla-cms\components\com_tags\src\Helper\RouteHelper.php:157 {
      Joomla\Component\Tags\Site\Helper\RouteHelper::_findItem($needles = null)^
      › $app      = Factory::getApplication();
      › $menus    = $app->getMenu('site');
      › $language = $needles['language'] ?? '*';
    }
    C:\wamp\www\joomla-cms\components\com_tags\src\Helper\RouteHelper.php:104 { …}
    C:\wamp\www\joomla-cms\plugins\finder\tags\tags.php:232 { …}
    C:\wamp\www\joomla-cms\administrator\components\com_finder\src\Indexer\Adapter.php:247 { …}
    C:\wamp\www\joomla-cms\libraries\src\Plugin\CMSPlugin.php:285 { …}
    C:\wamp\www\joomla-cms\libraries\vendor\joomla\event\src\Dispatcher.php:495 { …}
    C:\wamp\www\joomla-cms\libraries\src\Application\EventAware.php:111 { …}
    C:\wamp\www\joomla-cms\cli\finder_indexer.php:266 { …}
    C:\wamp\www\joomla-cms\cli\finder_indexer.php:194 { …}
    C:\wamp\www\joomla-cms\libraries\src\Application\CliApplication.php:241 { …}
    C:\wamp\www\joomla-cms\cli\finder_indexer.php:514 { …}
  }
}

Expected result AFTER applying this Pull Request

No errors.

Documentation Changes Required

@mbabker
Copy link
Contributor

mbabker commented Jul 7, 2020

This isn’t a proper fix. Last I knew the finder CLI wasn’t booting up the environment correctly and that still needs to be addressed (like making it a proper console command would be a good idea).

@SharkyKZ
Copy link
Contributor Author

SharkyKZ commented Jul 7, 2020

How is that going to fix the issue though? Console application doesn't have getMenu() method either.

@mbabker
Copy link
Contributor

mbabker commented Jul 7, 2020

Then there needs to be a type check.

The getMenu() method is only defined as part of CMSWebApplicationInterface so there should be a type check for that and an appropriate workaround if the type does not match. Pulling an application instance out of the container is a red flag.

Or, just skip going through the application and directly call AbstractMenu::getInstance(). Abusing Factory::getContainer() is just wrong though.

@SharkyKZ
Copy link
Contributor Author

SharkyKZ commented Jul 7, 2020

Switched to AbstractMenu::getInstance(). Thanks.

@SharkyKZ
Copy link
Contributor Author

SharkyKZ commented Jul 7, 2020

Any explanation on why getting it from the container is wrong?

@mbabker
Copy link
Contributor

mbabker commented Jul 7, 2020

Factory::getContainer() isn't intended to be a replacement for any existing factory singletons or static instance methods. Sooner or later, the right thing to do would be have dependencies correctly injected into services, which the container helps with orchestrating. So calls to Factory::getContainer() really should be few and far between, and if you're thinking about using it then maybe you need to be looking for another approach to whatever you're intending to accomplish.

Unfortunately, a lot of the core architecture still isn't in a great place to handle dependency injection for one reason or another (static methods accessing globals, classes randomly instantiated in places where there isn't a sane way to support injection, core still being designed around a lazy load paradigm meaning if you're trying to use something that doesn't originate out of the Joomla\CMS namespace you're stuck trying to load those services yourself in some form, etc.), so the container was added as a global in the factory to help write code that assists in the migration to a proper service oriented infrastructure.

@SharkyKZ
Copy link
Contributor Author

SharkyKZ commented Jul 8, 2020

Well, I just needed to get a specific application instance for now. Joomla\CMS\Factory::getApplication() no longer supports this in 4.0 and Joomla\CMS\Application\CMSApplication::getInstance() is deprecated with Joomla\CMS\Factory::getContainer()->get() as replacement so I used that.

@ceford
Copy link
Contributor

ceford commented Jul 8, 2020

I have tested this item ✅ successfully on 39a4193

Result Output:
Smart Search INDEXER

Starting Indexer
Setting up Smart Search plugins
Setup 31 items in 0.066 seconds.

  • Processed batch 1 in 0.476 seconds.
  • Skipping pause, as previous batch had a very low processing time (0.476s < 1s)
    Total Processing Time: 0.542 seconds.
    Peak memory usage: 14,680,064 bytes

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

@bonzani
Copy link

bonzani commented Jul 13, 2020

I have tested this item ✅ successfully on 39a4193


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

@SharkyKZ
Copy link
Contributor Author

RTC.


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 13, 2020
@richard67 richard67 merged commit 39f8389 into joomla:4.0-dev Jul 20, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 20, 2020
@richard67
Copy link
Member

Thanks!

@richard67 richard67 added this to the Joomla 4.0 milestone Jul 20, 2020
dgrammatiko added a commit to dgrammatiko/joomla-cms that referenced this pull request Jul 21, 2020
…outs

* '4.0-dev' of github.com:joomla/joomla-cms: (612 commits)
  [4.0] Smart Search: Fixing ordering, order direction and disabled button (joomla#29474)
  [4.0] Generate routed Modal links for iframes when not on the root (joomla#30007)
  [4.0] Get menu directly in com_tags menu route helper (joomla#30039)
  Remove collapse when resizing from mobile to desktop (joomla#30132)
  [4.0] Wrap component output in `main` element to make Cassiopeia more accessible (joomla#29870)
  [4.0] Webauthn gmp warning (joomla#29731)
  [4.0] Refactor to return early, remove if depths and throw NotAllowed (joomla#29694)
  [4.0] CLI help text (joomla#29811)
  Feature/draggable typo fixes (joomla#29987)
  [4.0] Removing unnecessary workaround in finder indexer (joomla#30037)
  [4.0] Optimizing Smart Search for larger content (joomla#30008)
  [4.0] Fix js ajax for pre update checker (joomla#29980)
  [4.0] Cassiopea: Fixing modals custom-select fields display (joomla#30097)
  [4.0][com_fields] Fix draggable sorting (joomla#30094)
  [4.0] Correct incorrect @return documentation (joomla#30092)
  [4.0] Menu items modal: adding missing filters (joomla#30087)
  short to long php open tags with echo (joomla#30089)
  Use new Toolbar (joomla#30085)
  [4.0] Center status/date created headers (joomla#29249)
  [4.0] Fix Cassiopea searchtools alignment in modals (joomla#30077)
  ...

# Conflicts:
#	administrator/components/com_templates/src/View/Template/HtmlView.php
#	installation/sql/postgresql/base.sql
#	libraries/src/Application/AdministratorApplication.php
#	libraries/src/Application/SiteApplication.php
sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
* Get application from container

* Get menu directly

* Remove import
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.

6 participants