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.

Sign up for GitHub

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

Jump to bottom

Fix ArgumentCountError on the TenantAssetsController #894

Merged
merged 5 commits into from
Jul 20, 2022
Merged

Fix ArgumentCountError on the TenantAssetsController #894

merged 5 commits into from
Jul 20, 2022

Conversation

megawubs
Copy link
Contributor

When no $path is provided in the tenant_asset(), the link that is generated does not contain the $path part, but in the route registration {path?} is optional. This results in an ArgumentCountError.

By making it default null it now returns an 404 as expected.

@megawubs megawubs changed the title Fix ArgumentCount exception on the TenantAssetsController Fix ArgumentCountError on the TenantAssetsController Jul 12, 2022
@stancl
Copy link
Member

stancl commented Jul 19, 2022

When no $path is provided in the tenant_asset()

What's the use case for this? Passing variables that may be empty to tenant_asset()?

@megawubs
Copy link
Contributor Author

When no $path is provided in the tenant_asset()

What's the use case for this? Passing variables that may be empty to tenant_asset()?

Yes, exactly that.

@stancl
Copy link
Member

stancl commented Jul 20, 2022 •
edited
Loading

I think the null should be explicitly handled in the method, we don't call the rest of the code when it's null.

Add an abort_if($path === null, 404) (not sure about arg order) above the try {} block

@megawubs
Copy link
Contributor Author

Done! ✅

@stancl stancl merged commit 747c192 into archtechx:3.x Jul 20, 2022
@stancl
Copy link
Member

stancl commented Jul 20, 2022

Thanks for the contribution!

@megawubs megawubs deleted the bugfix/empty-resource branch July 21, 2022 06:48
stancl added a commit that referenced this pull request Aug 22, 2022
@stancl @lukinovec
@megawubs @rootgog @abrardev99
Pull 3.x changes into master (#922)
931c76d 
* exclude master from CI

* Add space after 'up' in 'docker-compose up-d' (#900)

* Fix ArgumentCountError on the TenantAssetsController (#894)

* Fix ArgumentCount exception on the TenantAssetsController when no `$path` is provided

* CS

* CS

* Handle null case explicitly

* code style

Co-authored-by: Bram Wubs <bram@sibi.nl>
Co-authored-by: Samuel Štancl <samuel@archte.ch>

* Add support for nested tenant config override (#920)

* feat: add support for nested tenant config override

* test: ensure nested tenant values are mapped

* Update TenantConfigTest.php

Co-authored-by: lukinovec <lukinovec@gmail.com>
Co-authored-by: Bram Wubs <megawubs@users.noreply.github.com>
Co-authored-by: Bram Wubs <bram@sibi.nl>
Co-authored-by: George Bishop <email.georgebishop@gmail.com>
Co-authored-by: Abrar Ahmad <abrar.dev99@gmail.com>
stancl added a commit that referenced this pull request Aug 25, 2022
@erikgaal @stancl
@lukinovec @megawubs @abrardev99
[WIP] Use a lighter Docker image, use it in CI (#793)
ca2eefa 
* Use action services and setup-php in workflow

* Fix codecov

* exclude master from CI

* Add space after 'up' in 'docker-compose up-d' (#900)

* Fix ArgumentCountError on the TenantAssetsController (#894)

* Fix ArgumentCount exception on the TenantAssetsController when no `$path` is provided

* CS

* CS

* Handle null case explicitly

* code style

Co-authored-by: Bram Wubs <bram@sibi.nl>
Co-authored-by: Samuel Štancl <samuel@archte.ch>

* Improve Dockerfile and use it in CI

* Update Dockerfile

* mssql CI health check

* cache key

* Update ci.yml

* Update ci.yml

* Update composer.json

* register dumcommand when L8

* Update ci.yml

* Update composer.json

* Update composer.json

* Update composer.json

* wip

* removed extensions config and php version from matrix

* introduce php-cs-fixer issue for testing

* Fix code style (php-cs-fixer)

* install composer in Docker and remove setup-php step

* added pcov for coverage

* on master branch

* composer test command

* tests above services

* Update ci.yml

* Revert "register dumcommand when L8"

This reverts commit f165fc5.

* removed composer cache dependencies

Co-authored-by: Samuel Štancl <samuel.stancl@gmail.com>
Co-authored-by: lukinovec <lukinovec@gmail.com>
Co-authored-by: Bram Wubs <megawubs@users.noreply.github.com>
Co-authored-by: Bram Wubs <bram@sibi.nl>
Co-authored-by: Samuel Štancl <samuel@archte.ch>
Co-authored-by: Abrar Ahmad <abrar.dev99@gmail.com>
Co-authored-by: PHP CS Fixer <phpcsfixer@example.com>
stancl added a commit that referenced this pull request Feb 20, 2023
@lukinovec @stancl
@megawubs @rootgog @anburocky3 @wilsenhc @joelstein @saade
Add L10 support to 4.x (merge 3.x to master) (#1071)
e61a26d 
* exclude master from CI

* Add space after 'up' in 'docker-compose up-d' (#900)

* Fix ArgumentCountError on the TenantAssetsController (#894)

* Fix ArgumentCount exception on the TenantAssetsController when no `$path` is provided

* CS

* CS

* Handle null case explicitly

* code style

Co-authored-by: Bram Wubs <bram@sibi.nl>
Co-authored-by: Samuel Štancl <samuel@archte.ch>

* Add support for nested tenant config override (#920)

* feat: add support for nested tenant config override

* test: ensure nested tenant values are mapped

* fix: typo mistake (#954)

* [3.x] Add Vite helper for tenancy (#956)

* Add Vite helper for tenancy

* Move Vite bundler to an Optional Feature

* Rename to foundation vite

* Add ViteBundlerTest

* Add missing end of file

* Update tests

* remove unnecessary end() call

Co-authored-by: Samuel Štancl <samuel@archte.ch>

* rewrite ViteBundlerTest to phpunit syntax

* skip vite test in Laravel < 9

* convert ViteBundler to PHP 7 syntax

* remove import of nonexistent class in older Laravel versions

* remove import of Foundation\Vite in tests

* try to exclude Vite.php from coverage report

* remove typehint

* update channel name

* Cache crash fix (#1048)

* Don't prevent accessing missing Tenant attributes. (#1045)

* [3.x] L10 compatibility (#1065)

* Bump dependencies for Laravel 10

* Update GitHub Actions for Laravel 10

* ci: do not test L10 using PHP 7.3

* drop < L9 support

* use `dispatch_sync` instead of `dispatch_now`

* migrate phpunit configuration

* Update ci.yml

* drop laravel < 9 support

* misc L10 fixes, new docker image

* specify odbc version

* wip

* properly list php versions as strings

* minor changes

* Add `getValue($queryGrammar)` to raw query

* Clean up `isVersion8` code

* rewrite hasFailed assertion

* phpunit schema update

* Upgrade `doctrine/dbal`

---------

Co-authored-by: Samuel Štancl <samuel@archte.ch>
Co-authored-by: Samuel Štancl <samuel.stancl@gmail.com>
Co-authored-by: lukinovec <lukinovec@gmail.com>

* Update ci.yml

* Fix code style (php-cs-fixer)

* Update dependencies

* Change invade version

* Delete ViteBundlerTest

* Fix PHPStan error

* Delete PHPStan error ignore

* Fix CONTRIBUTING.md

* Delete ViteBundler remains

* Bring back ViteBundler

* Convert ViteBundlerTest to Pest

* Update ci.yml

---------

Co-authored-by: Samuel Štancl <samuel.stancl@gmail.com>
Co-authored-by: Bram Wubs <megawubs@users.noreply.github.com>
Co-authored-by: Bram Wubs <bram@sibi.nl>
Co-authored-by: Samuel Štancl <samuel@archte.ch>
Co-authored-by: George Bishop <email.georgebishop@gmail.com>
Co-authored-by: Anbuselvan Rocky <15264938+anburocky3@users.noreply.github.com>
Co-authored-by: Wilsen Hernández <13445515+wilsenhc@users.noreply.github.com>
Co-authored-by: Joel Stein <joel@mediatrix.digital>
Co-authored-by: Guilherme Saade <saade@outlook.com.br>
Co-authored-by: PHP CS Fixer <phpcsfixer@example.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@megawubs @stancl