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/fix: allow replacing of blade template namespaces #3167

Merged
merged 19 commits into from
Dec 20, 2021

Conversation

davwheat
Copy link
Member

Changes proposed in this pull request:

@imorland deserves the credit for this, but I'll happily take it (unless it doesn't work, then blame him!).

  • Allow replacing Blade template namespaces

This is, for example, needed for overriding the forum.blade.php file to inject custom HTML where needed, and not only where the headerHtml and footerHtml is included.

This PR will need tests! I'll try my best... 🙃

Reviewers should focus on:

Is this a feature or a bug fix? Replacing blade views used to be possible during beta, but I believe when the shift was made to extenders, this functionality was lost.

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)

@askvortsov1
Copy link
Sponsor Member

I'll want to see tests before reviewing, sorry.

Also, do ALL files in a namespace get replaced? That could be a lot of code duplication.

@SychO9
Copy link
Member

SychO9 commented Nov 19, 2021

This is, for example, needed for overriding the forum.blade.php file to inject custom HTML where needed, and not only where the headerHtml and footerHtml is included.

You can still replace that view using the Frontend extender's content method, example of replacing the admin.blade.php:
https://github.com/afrux/asirem/blob/main/extend.php#L38

Now that probably doesn't/can't be applied to other views such as errors, but I reckon there's probably a better way to go about replacing a specific view than replacing a whole namespace? especially if

Also, do ALL files in a namespace get replaced? That could be a lot of code duplication.

@askvortsov1
Copy link
Sponsor Member

We'll want to support both prependNamespace and replaceNamespace, although we might want to start with just prepend since that's more useful.

src/Extend/View.php Outdated Show resolved Hide resolved
tests/integration/extenders/ViewTest.php Outdated Show resolved Hide resolved
src/Extend/View.php Outdated Show resolved Hide resolved
src/Extend/View.php Outdated Show resolved Hide resolved
src/Extend/View.php Outdated Show resolved Hide resolved
tests/integration/extenders/ViewTest.php Outdated Show resolved Hide resolved
We only really need prepend.
Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Sorry, a few more requests:

Can we add a test case showing that you can override a view used by flarum, such as the homepage, in a full integration test?

Maybe we should rename prepend on the extender to override or extendNamespace?

@davwheat
Copy link
Member Author

Out of those options, I prefer extendNamespace because we aren't technically overriding even if it seems that way. It also gives us the ability to easily slot in overrideNamespace in the future if the need arises.

@davwheat davwheat merged commit 4ad961c into master Dec 20, 2021
@davwheat davwheat deleted the dw/allow-blade-overrides branch December 20, 2021 08:56
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.

4 participants