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

[Fix] Search Module does not display in the error.php page #18375

Merged
merged 1 commit into from Oct 23, 2017

Conversation

infograf768
Copy link
Member

@infograf768 infograf768 commented Oct 20, 2017

Testing Instructions

Display the error page with Protostar or Beez with a link like
mydomain.com/whatever

Expected result

The search module should display

Actual result

It does not not display

Patch and test again

You should get for Protostar
screen shot 2017-10-20 at 18 01 59

and for Beez

screen shot 2017-10-20 at 18 03 40

@infograf768
Copy link
Member Author

@mbabker

@brianteeman
Copy link
Contributor

But what changes in the core have made this problem as this has worked for years. Surely that is the "bug" to find and fix. This is just a band aid to mask the "bug"

@csthomas
Copy link
Contributor

csthomas commented Oct 20, 2017

This PR is now only for beez3 and protostar templates.

[UPDATED]

By replacing search to mod_search you will always get some module, if mod_search is disabled or only set for some menu items then you get dummy object but render mod_search without any parameters from db.

@mbabker
Copy link
Contributor

mbabker commented Oct 20, 2017

The $this->getBuffer() change is because only the HTML document type really has the concept of module positions, error pages use the Error document type and that doesn't. So you have to manually render the module.

As for search versus mod_search, as I said in the other issue it shouldn't matter which you do as long as the module load process goes correctly. If it doesn't go correctly or plugins are messing with the result set, it's possible the name isn't there (though since the object is an unkeyed stdClass and not a proper data object in theory the module isn't reliable either).

@csthomas
Copy link
Contributor

csthomas commented Oct 20, 2017

The part with removing $this->getBuffer() is OK, I would not replace search by mod_search

@csthomas
Copy link
Contributor

I want to say that if the module will be disabled/removed then it will still be displayed on error page.

@mbabker
Copy link
Contributor

mbabker commented Oct 20, 2017

I want to say that if the module will be disabled/removed then it will still be displayed on error page.

Well, this very easily balloons into a bigger patch. Because:

  • The module should be configurable based on what search your site uses (right now it's hardcoded)
  • If you don't use a search engine in your site, then the text should not hint towards searching

For that first part, what we do in the joomla.org template is add a parameter to the template style to choose between mod_finder and mod_search to at least let the module use the site's active search component, we didn't account for the no search scenario though and to deal with a "well this site uses a custom search" thing you'd have to hook the form through the normal plugin events to add your option.

For what this is right now, it's fine I would say.

@csthomas
Copy link
Contributor

I have tested this item ✅ successfully on b691428


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

1 similar comment
@alikon
Copy link
Contributor

alikon commented Oct 21, 2017

I have tested this item ✅ successfully on b691428


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

@ghost
Copy link

ghost commented Oct 21, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 21, 2017
@infograf768 infograf768 added this to the Joomla 3.8.2 milestone Oct 21, 2017
@mbabker mbabker merged commit 4a536af into joomla:staging Oct 23, 2017
@joomla-cms-bot joomla-cms-bot added PR-staging and removed RTC This Pull Request is Ready To Commit labels Oct 23, 2017
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

6 participants