-
Notifications
You must be signed in to change notification settings - Fork 254
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
Make internal PHP functionality private #341
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…y@filter return the Ziggy instance applyFilters() and group() aren't very useful on their own because they only return the list of routes, and the don't provide any convenience—*using* either of them requires setting config options or passing arguments, so with filter() now returning a Ziggy instance, they aren't necessary
jakebathman
approved these changes
Oct 23, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to merge 👍
bakerkretzmar
added a commit
that referenced
this pull request
Nov 6, 2020
* Add upgrade guide * Wip * Wip * Update GitHub actions to not run on Markdown files * Wip * Formatting * Add changes in #337 * Add anchor links * Add changes in #338 * Add changes in #341, formatting * Wip * Update usage examples in Readme * Move CSP section down * LICENSE.md → LICENSE * Wip * Wip * Formatting * Add new features section to Upgrading * Prep Changelog headers and tags for v1 * Move current() with query fix to Fixed section * Add entries for #334 and #344 to Upgrading * Add Upgrading entry for check() being deprecated * Add #345 to Upgrading * Wip on Readme * JavaScript → Javascript * Wording/formatting * Update Readme: - Move Usage above Setup - Remove 'basic setup' section about @routes directive, it's covered in Installation - Wording fixes - Add note about boolean encoding (see #345) - Remove old 'Artisan Command' section * Formatting * Formatting and wording * Javascript → JavaScript * Remove unused heading link * Add section under 'Other' for setting up an API endpoint to return routes, link to that from SPA and JS sections * Fix wording re: watching files * :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes some methods and makes a few others private in Ziggy's PHP API. These methods were either for internal use or provided no real functionality of their own.
Notable changes
filter()
method on theZiggy
class is now 'fluent' and returns an instance ofZiggy
instead of a collection of routes.Other changes
nameKeyedRoutes()
andresolveBindings()
methods on theZiggy
class, which are for internal use, are now private.generate()
method on theCommandRouteGenerator
is now private.applyFilters()
andgroup()
methods on theZiggy
class are now private. This might seem like a significant change, but it isn't. Both of these methods returned an array of Ziggy-formatted route definitions, which isn't particularly useful on its own, but more importantly, they were both redundant—Ziggy already uses these methods internally to filter its routes, so callinggroup()
orapplyFilters()
directly has the same effect as configuring or passing the same options before instantiating a Ziggy object. The easiest way to understand this is to look at the changes to the tests in this PR: all of the data returned byapplyFilters()
andgroup()
was already available in thenamedRoutes
key of the array that Ziggy returns.Cleanup
appendRouteToList()
andisListedAs()
methods, and code referencing them. These were both unused since theRouter
Facade macro was removed in Remove Route facade macros #306.except()
andonly()
methods that didn't return anything and were already being called automatically when retrieving Ziggy's config. This functionality was inlined into theapplyFilters()
method.