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.

Sign up for GitHub

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

Jump to bottom

Add Target column to the LinkFieldIndex table #16380

Closed
wants to merge 7 commits into from
Closed

Add Target column to the LinkFieldIndex table #16380

wants to merge 7 commits into from

Conversation

MikeAlhayek
Copy link
Member

Fix #16378

@MikeAlhayek MikeAlhayek requested a review from Piedone June 27, 2024 15:39
Piedone
Piedone requested changes Jun 27, 2024
View reviewed changes
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only briefly checked out the code, but I'll be able to properly test and asses it once I'm back from holiday, next Tuesday.

src/OrchardCore.Modules/OrchardCore.ContentFields/Indexing/SQL/Migrations.cs Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.ContentFields/Indexing/SQL/LinkFieldIndexProvider.cs Show resolved Hide resolved
This was referenced Jul 1, 2024
Open
Closed
@MikeAlhayek MikeAlhayek requested a review from Piedone July 2, 2024 16:04
@sebastienros
Copy link
Member

So nobody agrees with my comments ? (#16326 (comment)) we just accept anything in the index? (or graphql, sql index, ... why stop there). Even when there is an obvious alternative: adding fields using existing types.

Piedone
Piedone approved these changes Jul 2, 2024
View reviewed changes
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is OK now, but yeah, perhaps we should not have this in the index, in the first place.

@MikeAlhayek
Copy link
Member Author

closing this in favor of #16397

@MikeAlhayek MikeAlhayek closed this Jul 2, 2024
@Piedone Piedone mentioned this pull request Jul 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Piedone Piedone Piedone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rebuilding Lucene Index fails
3 participants
@MikeAlhayek @sebastienros @Piedone