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

feat: frontend content flexible order priorities #3765

Merged
merged 4 commits into from
Oct 29, 2023

Conversation

garygreen
Copy link
Contributor

@garygreen garygreen commented Mar 17, 2023

This allows access to page/controller loaded content when using the Frontend content extender - which allows developers to fully customise the document programatically, e.g. able to see Discussion, Tag, etc.

Previous to this PR this was not possible:

   (new Extend\Frontend('forum'))
        ->content(function(Document $document, Request $request) {
                  // Not possible to get the Discussion, or Tag, or anything that the controller loaded here from the `document`
                 $document->title; // always null, despite viewing a Discussion page.
                 dd($document);
        });

Before - $document - notice title is missing
image

After - notice Discussion information is filled on the document and are able to access the model data.

image

Use Cases

This has many - one we are looking at is to add a <meta> SEO noindex tag to our Discussion page if we consider the posts there to have "thin content" value. With this PR, it's easily possible to get acccess to the Discussion document and relevant posts. Prior to this PR, it's not easily possible without manually loading the Discussion model, etc.

Reviewers should focus on:
This potentially introduces a breaking behavioural change if people were relying on the old behaviour. I think it's a minor change though.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@garygreen garygreen requested a review from a team as a code owner March 17, 2023 03:33
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR!

This would fix flarum/issue-archive#199

I don't know yet whether this is BC compatible or not, so need to check 🤔

framework/core/src/Forum/ForumServiceProvider.php Outdated Show resolved Hide resolved
@garygreen
Copy link
Contributor Author

garygreen commented Mar 25, 2023

@SychO9 It does indeed fix flarum/issue-archive#199 - I hadn't seen that issue up until now, glad there is a discussion for this. This is a simple PR that allows devs to have complete final control over the state of the Document.

It might be a BC like you say, as some may rely on the current behaviour of Document missing lots of values, and expecting it to override stuff that was set. That is quite unlikely though, imo. If it is a concern then it can be documented in an upgrade guide, or this could target 2.0 (how far away is that? hopefully not too far because we need this PR!) 😂

@SychO9 SychO9 changed the base branch from main to 2.x May 27, 2023 17:41
@SychO9 SychO9 changed the title Fix being able to extend and customise frontend controller-specific content feat: frontend content flexible order priorities Sep 15, 2023
@SychO9 SychO9 added this to the 2.0 milestone Sep 15, 2023
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

I have added priorities to the PR so that it's a lot more flexible, any developer can now choose to add content before any other content or after using priority numbers.

@SychO9 SychO9 requested review from a team and imorland September 15, 2023 10:07
@SychO9 SychO9 merged commit 7c885c7 into flarum:2.x Oct 29, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants