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

Correctly highlight phrases in the search results (alternative) #1274

Closed
wants to merge 1 commit into from

Conversation

@leofeyer
Copy link
Member

leofeyer commented Feb 3, 2020

This is an alternative approach to #1270 which does not require to pass a seventh argument via reference. It adds a public overwrite() method to the Contao\Database\Result class though.

@contao/developers Please let me know which variant you prefer.

@leofeyer leofeyer added the defect label Feb 3, 2020
@leofeyer leofeyer added this to the 4.4 milestone Feb 3, 2020
@leofeyer leofeyer requested a review from contao/reviewers Feb 3, 2020
@leofeyer leofeyer self-assigned this Feb 3, 2020
@bytehead

This comment has been minimized.

Copy link
Member

bytehead commented Feb 3, 2020

👍 for this (#1274).

@leofeyer leofeyer force-pushed the bugfix/highlight-search-results-alternative branch from ac64f0e to 02d3975 Feb 3, 2020
@fritzmg
fritzmg approved these changes Feb 3, 2020
Copy link
Contributor

aschempp left a comment

I think it would be majorly more flexible to allow creating a Database\Result from array instead of a DoctrineStatement. The only necessary change would be to allow the first argument to be an array or a doctrine statement, and initialize the records accordingly.

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Feb 4, 2020

Not sure if that is a good idea. The Result class is actually supposed to work directly with the PHP resource object, so we do not have to load the entire result set into memory (you probably remember how it used to work in Contao 3).

When we switched to Symfony and Doctrine in Contao 4, working with the PHP resource object was no longer possible, so we bit the bullet and now we import the entire result set into memory. However, there is also a @todo comment "Try to find a solution that works without fetchAll()".

So if we are sure that there will never be a solution that works without fetchAll(), we might as well allow to initialize the Result object from an array. But if we are not, we should keep the type hint.

@contao/developers Are we sure about it or are we not? 😉

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Feb 4, 2020

If this one works reliably (which I cannot verify), I'm in favor of this variant as well.

@ausi

This comment has been minimized.

Copy link
Member

ausi commented Feb 4, 2020

Created a pull request which removes fetchAll() from the Result class and allows creating a Result from an array: #1287

@fritzmg

This comment has been minimized.

Copy link
Collaborator

fritzmg commented Feb 5, 2020

@contao/developers Are we sure about it or are we not? 😉

imho fixing the search highlighting is not important enough to disregard that todo 😉

leofeyer pushed a commit that referenced this pull request Feb 14, 2020
Description
-----------

This is related to #1274 (comment) an implementation of the `Result` class that doesn’t use `fetchAll` and only copies the data for used/read rows.

But I think for the use case in #1274 we should enable the initialization from an `array` instead of the `overwrite()` method. I’ve added this already to this PR.

Commits
-------

f4ba0bf Don’t use fetchAll and enable initialization from array
ac75d26 Fix fetchRow behavior
bc24f17 Remove initialization from array
88ef61d Fix the coding style
4358c18 Remove arrCache to save memory
a29bd2c Optimize memory usage for single row results
2371b3e Fix the coding style
leofeyer pushed a commit to contao/core-bundle that referenced this pull request Feb 14, 2020
Description
-----------

This is related to contao/contao#1274 (comment) an implementation of the `Result` class that doesn’t use `fetchAll` and only copies the data for used/read rows.

But I think for the use case in #1274 we should enable the initialization from an `array` instead of the `overwrite()` method. I’ve added this already to this PR.

Commits
-------

f4ba0bfe Don’t use fetchAll and enable initialization from array
ac75d263 Fix fetchRow behavior
bc24f171 Remove initialization from array
88ef61d7 Fix the coding style
4358c182 Remove arrCache to save memory
a29bd2c0 Optimize memory usage for single row results
2371b3e2 Fix the coding style
@leofeyer leofeyer modified the milestones: 4.4, 4.9 Feb 14, 2020
@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Feb 14, 2020

Closed in favor of #1335.

@leofeyer leofeyer closed this Feb 14, 2020
@leofeyer leofeyer deleted the bugfix/highlight-search-results-alternative branch Feb 14, 2020
@leofeyer leofeyer removed this from the 4.9 milestone Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.