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

[4][cli] finder:index #38222

Closed
wants to merge 1 commit into from
Closed

[4][cli] finder:index #38222

wants to merge 1 commit into from

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Jul 4, 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

@brianteeman
Copy link
Contributor

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.

1 similar comment
@viocassel
Copy link
Contributor

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.

@alikon
Copy link
Contributor Author

alikon commented Jul 4, 2022

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

public function getTemplate($params = false)

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 4, 2022
@wilsonge
Copy link
Contributor

wilsonge commented Jul 4, 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).

@richard67
Copy link
Member

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?

@wilsonge
Copy link
Contributor

wilsonge commented Jul 4, 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?

@Fedik
Copy link
Member

Fedik commented Jul 4, 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 :)

@alikon
Copy link
Contributor Author

alikon commented Jul 4, 2022

@wilsonge this is the call stack
image

@wilsonge
Copy link
Contributor

wilsonge commented Jul 4, 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.

@Hackwar
Copy link
Member

Hackwar commented Jul 4, 2022

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

@Fedik
Copy link
Member

Fedik commented Jul 4, 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 :)

@wilsonge
Copy link
Contributor

wilsonge commented Jul 4, 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

@roland-d
Copy link
Contributor

roland-d commented Jul 4, 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?

@wilsonge
Copy link
Contributor

wilsonge commented Jul 4, 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

@alikon
Copy link
Contributor Author

alikon commented Jul 5, 2022

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

@roland-d
Copy link
Contributor

roland-d commented Jul 5, 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.

@brianteeman
Copy link
Contributor

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

@roland-d
Copy link
Contributor

roland-d commented Jul 5, 2022

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

@roland-d roland-d closed this Jul 5, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 5, 2022
@roland-d
Copy link
Contributor

roland-d commented Jul 5, 2022

Thanks everybody for your work on this.

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.

None yet

9 participants