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

View Extender (add namespace) #2134

Merged
merged 9 commits into from Jul 17, 2020

Conversation

askvortsov1
Copy link
Sponsor Member

@askvortsov1 askvortsov1 commented Apr 24, 2020

Fixes part of #1891

Changes proposed in this pull request:
Add view extender with addNamespace method. I called it this instead of just ->namespace() in case we want to support prependNamespace, replaceNamespace, etc

Reviewers should focus on:
Do we want to include replaceNamespace here? I think that might be useful. EDIT: Not yet. Flarum's namespaces are loaded after extenders do their thing, so it wouldn't work there, hence, we should hold off on that for now.

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

@franzliedke
Copy link
Contributor

What do you think about naming the class ViewNamespace and the method add()? I just went through all existing extenders and this would be more in line with them. All of them have very concise method names, and add() is only used when other operations like remove() exist as well.

Hmm... 🤔 ... do we really need other operations? We barely use the concept of namespaces in core, so there's nothing there to replace. And I don't want to suggest removing / replacing other extensions' namespaces is a good thing (or important enough to deal with in one of the bundled extenders).

tests/integration/extenders/ViewTest.php Outdated Show resolved Hide resolved
tests/integration/resources/views/test.blade.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
@franzliedke franzliedke merged commit b5e891d into flarum:master Jul 17, 2020
franzliedke added a commit that referenced this pull request Jul 17, 2020
Not all requests need this factory, so there is no need to
instantiate one and load the required files.

Refs #1891, #2134.
franzliedke added a commit that referenced this pull request Jul 17, 2020
As discussed in my initial review, it seems unlikely that we need
the ability to remove (or otherwise modify) namespaces again.
Therefore, it seems more consistent with other extenders to go
for a "View" extender with a "namespace" method.

Sorry for the back and forth. ;)

Refs #1891, #2134.
@franzliedke
Copy link
Contributor

I took care of the lazy extending (requested in #2134 (comment)) and the renaming (discussed in #2134 (comment)) in master.

Thanks for the PR! 🙌

franzliedke added a commit to flarum/mentions that referenced this pull request Jul 17, 2020
franzliedke added a commit to flarum/subscriptions that referenced this pull request Jul 17, 2020
franzliedke added a commit to flarum/tags that referenced this pull request Jul 17, 2020
askvortsov1 pushed a commit to flarum/mentions that referenced this pull request Mar 11, 2022
askvortsov1 pushed a commit to flarum/subscriptions that referenced this pull request Mar 11, 2022
askvortsov1 pushed a commit to flarum/tags that referenced this pull request Mar 11, 2022
askvortsov1 pushed a commit to flarum/mentions that referenced this pull request May 10, 2022
askvortsov1 pushed a commit to flarum/subscriptions that referenced this pull request May 10, 2022
askvortsov1 pushed a commit to flarum/tags that referenced this pull request May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants