? ? Pending

User tests: Successful: Unsuccessful:

avatar alikon
alikon
4 Jul 2022

Pull Request for Issue #38214 .

Summary of Changes

get the Template in the Console Application (even if not needed) but like API application

Testing Instructions

from cli run
php cli/joomla.php finder:index

Actual result BEFORE applying this Pull Request

In PluginHelper.php line 46: Call to undefined method Joomla\CMS\Application\ConsoleApplication::getTemplate()

Expected result AFTER applying this Pull Request

finder indexer works

avatar alikon alikon - open - 4 Jul 2022
avatar alikon alikon - change - 4 Jul 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 4 Jul 2022
Category Libraries
avatar brianteeman brianteeman - test_item - 4 Jul 2022 - Tested successfully
avatar brianteeman
brianteeman - comment - 4 Jul 2022

I have tested this item successfully on 19e0d03


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

avatar viocassel viocassel - test_item - 4 Jul 2022 - Tested successfully
avatar viocassel
viocassel - comment - 4 Jul 2022

I have tested this item successfully on 19e0d03


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

avatar alikon
alikon - comment - 4 Jul 2022

@SharkyKZ the same method is in the API application as well

public function getTemplate($params = false)

avatar richard67 richard67 - change - 4 Jul 2022
Status Pending Ready to Commit
Labels Added: ?
avatar richard67
richard67 - comment - 4 Jul 2022

RTC


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

avatar wilsonge
wilsonge - comment - 4 Jul 2022

This one's a bit different to the api because we can check whether the application implements CMSApplicationInterface instead of CMSWebApplicationInterface (although in truth obviously would be better if the api didn't have that dummy getTemplate method).

avatar richard67
richard67 - comment - 4 Jul 2022

This one's a bit different to the api because we can check whether the application implements CMSApplicationInterface instead of CMSWebApplicationInterface (although in truth obviously would be better if the api didn't have that dummy getTemplate method).

@wilsonge So should I revert RTC and the PR needs to be changed or replaced by a new one?

avatar wilsonge
wilsonge - comment - 4 Jul 2022

I'm not sure. it depends how intrenched the usage is. Like I don't see PluginHelper::getLayoutPath being called by the finder plugins themselves. So what is actually calling this function and what's the effect of it looking for the system template?

avatar Fedik
Fedik - comment - 4 Jul 2022

This is a general issue in our override system, which is require Template (it not only about Finder).
Plugin may want to render some tmpl or layout in CLI/API Application context.

Defaulting to 'system' template will work, but it just hide the issue.
The fix is okay, as long as we do not have a better idea what to do here :)

avatar alikon
alikon - comment - 4 Jul 2022

@wilsonge this is the call stack
image

avatar wilsonge
wilsonge - comment - 4 Jul 2022

OK So comes from this #36747 PR which is a new 4.2 feature. So first of all that's caused a regression. Is this CLI working in 4.1?

Second of all that PR is setting prepareValue to true. But this is presumably going to index different based on whether it's frontend or backend driven edits causing the index because the active template will be different. Seems like that entire feature is very broken to me? Beyond just the CLI.

Tagging @Hackwar for insight as he wrote the code and might be able to give an understanding and @roland-d because looks like 4.2 currently is causing a regression.

avatar Hackwar
Hackwar - comment - 4 Jul 2022

I haven't written the CLI part here. I would have to look into this more deeply.

avatar Fedik
Fedik - comment - 4 Jul 2022

It not a Finder issue, it "Template Override" issue.
The same will happen if call LayoutHelper::render('foo.bar') in CLI context.
If we fix it, then a rest will work automatically :)

avatar wilsonge
wilsonge - comment - 4 Jul 2022

I haven't written the CLI part here. I would have to look into this more deeply.

This isn't a purely CLI issue. Your rendering the custom field on index - which uses the active template. Not the frontend template. In the CLI that crashes but even if that update comes from the backend seems dodgy to me

avatar roland-d
roland-d - comment - 4 Jul 2022

@wilsonge Thanks for the heads-up.

So first of all that's caused a regression. Is this CLI working in 4.1?

It is not a regression I guess because it fails with the same error on Joomla 4.1.5 for me.

$ php cli/joomla.php finder:index
Finder Indexer
==========================

 Starting Indexer
 Setting up Smart Search plugins
PHP Notice:  Undefined index: HTTP_HOST in .../j4/libraries/src/Uri/Uri.php on line 103
 Setup 127 items in 0.177 seconds.

In image.php line 57:
                                                                                     
  Call to undefined method Joomla\CMS\Application\ConsoleApplication::getTemplate()  
$ php cli/joomla.php 
Joomla! 4.1.5 (debug: Yes)

Regression or not, this is something we should fix, ideally before the last beta 3 release tomorrow.

@Hackwar Do you have time to look into this?

avatar wilsonge
wilsonge - comment - 4 Jul 2022

OK So sat down with the dev laptop tonight and actually tried doing some reproduction steps and debugging. First of all - with latest 4.2-dev and the test data set installed I can index from CLI just fine.

If I then create a custom field and make it searchable it fails. So i'm very confident in 4.2-dev it's related to that fields PR (at least for now - if it fails in 4.1-dev then possible there might be other scenarios we have to look at too). I can't install the same dataset there (because I run 8.1 locally and 4.1.5 doesn't have the PHP8.1 fixes for the test sample set)

4.2-dev:

Screenshot 2022-07-04 at 23 01 30

4.1.5:

Screenshot 2022-07-04 at 23 42 34

I think that PR needs reverting if it's not fixed because:

  1. Your going to get different results depending on if you are frontend or backend editing (because it may or may not load the correct template overrides for a custom field).
  2. I don't think it should work (searchable) for things like the media field. It certainly doesn't at the moment anyhow and I'm not sure we should show people the option
  3. If you make a subform field searchable (not such a totally crazy thing to do) then it hard crashes the index process.

Screenshot 2022-07-04 at 23 39 52

avatar alikon
alikon - comment - 5 Jul 2022

@roland-d with 4.1.5 stable it works well for me under the same settings for 4.2 (blog sample data)
image

avatar roland-d
roland-d - comment - 5 Jul 2022

Thank you @wilsonge for your detailed response. I agree, at this point we should revert this PR and take some time to further polish it.

@alikon Perhaps it comes from 3PD custom fields, I did not check why it fails, just that it fails.

This PR can be closed I guess and the final solution should be in the new PR for indexing custom fields.

avatar brianteeman
brianteeman - comment - 5 Jul 2022

so you will be reverting #36747 and closing this and waiting for a new pr

avatar roland-d
roland-d - comment - 5 Jul 2022

@brianteeman That is correct, a new PR needs to be made and give @Hackwar some time to look into this.

avatar roland-d roland-d - change - 5 Jul 2022
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2022-07-05 13:03:59
Closed_By roland-d
Labels Added: ?
avatar roland-d roland-d - close - 5 Jul 2022
avatar roland-d
roland-d - comment - 5 Jul 2022

Thanks everybody for your work on this.

Add a Comment

Login with GitHub to post a comment