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 canonical links to news and events #6215

Merged
merged 20 commits into from Jan 11, 2024
Merged

Add canonical links to news and events #6215

merged 20 commits into from Jan 11, 2024

Conversation

aschempp
Copy link
Member

Implements #5891

@aschempp aschempp added this to the 5.3 milestone Jul 14, 2023
@aschempp aschempp requested a review from a team July 14, 2023 13:05
@aschempp aschempp self-assigned this Jul 14, 2023
@aschempp aschempp mentioned this pull request Jul 14, 2023
Closed
@Toflar Toflar added the up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. label Aug 28, 2023
@Toflar
Copy link
Member

Toflar commented Oct 10, 2023

This PR also has the potential to fix the following problem: You can currently place the news/event reader on multiple root pages and have it output the same news entry creating duplicate content.

I know this is not directly possible with core modules, because you cannot override the target reader page with the news list/event list modules but you can easily do that with an extension. You could achieve the same situation with core-only when manually linking to a given news entry with the hyperlink element for example.

In any case: I think it would be smart if both, the calendar and news reader archives would have a new checkbox to activate generating a default canonical link to the configured archive reader page. This would then be overridden with the configurations per entry as in this PR. So you'd have:

  1. If there's a canonical setting in the news/calendar entry -> canonical accordingly
  2. If there's the checkbox is set in the news/calendar archive -> canonical to the forward page configured in the archive (might be just pointint to itself but can help solve cross-domain issues)
  3. If nothing is configured -> canonical to itself as we already have today.

@leofeyer
Copy link
Member

As discussed in the call, we want to add a HtmlHeadBagAccessor class in a separate pull request and then inject that instead of the insert tag service.

@leofeyer leofeyer removed the up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. label Nov 16, 2023
@aschempp aschempp mentioned this pull request Nov 16, 2023
Closed
@aschempp
Copy link
Member Author

As discussed in the call, we want to add a HtmlHeadBagAccessor class in a separate pull request and then inject that instead of the insert tag service.

I have simply duplicated the current page code in 7e11216 now. We can refactor that once we have #6547 (comment)

This PR also has the potential to fix the following problem: You can currently place the news/event reader on multiple root pages and have it output the same news entry creating duplicate content.

I know this is not directly possible with core modules, because you cannot override the target reader page with the news list/event list modules but you can easily do that with an extension. You could achieve the same situation with core-only when manually linking to a given news entry with the hyperlink element for example.

In any case: I think it would be smart if both, the calendar and news reader archives would have a new checkbox to activate generating a default canonical link to the configured archive reader page. This would then be overridden with the configurations per entry as in this PR. So you'd have:

  1. If there's a canonical setting in the news/calendar entry -> canonical accordingly
  2. If there's the checkbox is set in the news/calendar archive -> canonical to the forward page configured in the archive (might be just pointint to itself but can help solve cross-domain issues)
  3. If nothing is configured -> canonical to itself as we already have today.

implemented in 0646879

@aschempp
Merge remote-tracking branch 'upstream/5.x' into feature/news-event-c…
6f33fe9 
…anonical

# Conflicts:
#	calendar-bundle/contao/models/CalendarEventsModel.php
#	calendar-bundle/contao/models/CalendarModel.php
#	news-bundle/contao/models/NewsArchiveModel.php
#	news-bundle/contao/models/NewsModel.php
@aschempp
CS
daff855
@aschempp aschempp requested a review from Toflar November 24, 2023 16:46
Toflar
Toflar previously approved these changes Nov 27, 2023
View reviewed changes
@leofeyer leofeyer requested a review from ausi November 27, 2023 11:41
ausi
ausi previously approved these changes Nov 27, 2023
View reviewed changes
fritzmg
fritzmg previously approved these changes Nov 27, 2023
View reviewed changes
Toflar
Toflar previously approved these changes Dec 31, 2023
View reviewed changes
ausi
ausi requested changes Jan 2, 2024
View reviewed changes
calendar-bundle/contao/modules/ModuleEventReader.php Outdated Show resolved Hide resolved
news-bundle/contao/modules/ModuleNewsReader.php Outdated Show resolved Hide resolved
@leofeyer leofeyer changed the title Add canonical link to news and events Add canonical links to news and events Jan 3, 2024
@aschempp @leofeyer @ausi
Apply suggestions from code review
a5c8c4e 
Co-authored-by: Leo Feyer <1192057+leofeyer@users.noreply.github.com>
Co-authored-by: Martin Auswöger <martin@auswoeger.com>
aschempp and others added 5 commits January 5, 2024 08:19
@aschempp @ausi
Update news-bundle/contao/modules/ModuleNewsReader.php
8a22c15 
Co-authored-by: Martin Auswöger <martin@auswoeger.com>
@aschempp
Revert changes
adf1356
@aschempp
Merge remote-tracking branch 'upstream/5.x' into feature/news-event-c…
f30250f 
…anonical

# Conflicts:
#	calendar-bundle/contao/dca/tl_calendar_events.php
#	calendar-bundle/contao/dca/tl_module.php
#	news-bundle/contao/dca/tl_module.php
#	news-bundle/contao/dca/tl_news.php
#	news-bundle/contao/languages/en/tl_news_archive.xlf
@aschempp
Revert unrelated changes
71bff0a
@aschempp
CS
0ea54a1
@aschempp aschempp requested a review from ausi January 5, 2024 07:30
Toflar
Toflar previously approved these changes Jan 5, 2024
View reviewed changes
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Just wondering why it's news_keepCanonical - is that not the case for any other reader module (including the adjusted ModuleEventReader here?).

ausi
ausi previously approved these changes Jan 5, 2024
View reviewed changes
@aschempp
Copy link
Member Author

aschempp commented Jan 8, 2024

Just wondering why it's news_keepCanonical - is that not the case for any other reader module (including the adjusted ModuleEventReader here?).

It could be, yes. But every module needs to add its own field, because it does not know if it already exists. And the labels could be different per field.

@Toflar
Copy link
Member

Toflar commented Jan 8, 2024

Ah I missed cal_keepCanonical. All good then!

fritzmg
fritzmg previously approved these changes Jan 8, 2024
View reviewed changes
leofeyer
leofeyer requested changes Jan 9, 2024
View reviewed changes
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

The migrations need to be removed (see #6215 (comment)).

@leofeyer leofeyer dismissed stale reviews from fritzmg, Toflar, and ausi via c9b1f5f January 9, 2024 16:52
@aschempp
Copy link
Member Author

aschempp commented Jan 9, 2024

How about we merge as is and open a new issue to remove the migrations? 🙃 should be the same, except that we won't forget them?

@aschempp aschempp added the up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. label Jan 11, 2024
@leofeyer leofeyer merged commit c94a2da into contao:5.x Jan 11, 2024
17 checks passed
@leofeyer
Copy link
Member

Thank you @aschempp.

@aschempp aschempp deleted the feature/news-event-canonical branch January 18, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofeyer leofeyer leofeyer approved these changes

@ausi ausi ausi left review comments

@Toflar Toflar Toflar left review comments

@fritzmg fritzmg fritzmg left review comments

Assignees

@aschempp aschempp

Labels
feature up for discussion Issues and PRs which will be discussed in our monthly Mumble calls.
Projects
None yet
Milestone
5.3
Development

Successfully merging this pull request may close these issues.

rel-canonical for items of events and news
5 participants
@aschempp @Toflar @leofeyer @fritzmg @ausi