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

Make internal PHP functionality private #341

Merged
merged 10 commits into from
Oct 24, 2020
Merged

Conversation

bakerkretzmar
Copy link
Collaborator

@bakerkretzmar bakerkretzmar commented Oct 15, 2020

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

  • The filter() method on the Ziggy class is now 'fluent' and returns an instance of Ziggy instead of a collection of routes.

Other changes

  • The nameKeyedRoutes() and resolveBindings() methods on the Ziggy class, which are for internal use, are now private.
  • The generate() method on the CommandRouteGenerator is now private.
  • The applyFilters() and group() methods on the Ziggy 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 calling group() or applyFilters() 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 by applyFilters() and group() was already available in the namedRoutes key of the array that Ziggy returns.

Cleanup

  • Removed the appendRouteToList() and isListedAs() methods, and code referencing them. These were both unused since the Router Facade macro was removed in Remove Route facade macros #306.
  • Removed the except() and only() methods that didn't return anything and were already being called automatically when retrieving Ziggy's config. This functionality was inlined into the applyFilters() method.

…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
Copy link
Collaborator

@jakebathman jakebathman left a 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 bakerkretzmar merged commit b0446a4 into develop Oct 24, 2020
@bakerkretzmar bakerkretzmar deleted the jbk/php-private branch October 24, 2020 00:35
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants