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

Fixed #10224: fix route names for optimize command #15082

Merged
merged 2 commits into from
Jul 13, 2024

Conversation

dbakan
Copy link
Contributor

@dbakan dbakan commented Jul 12, 2024

Description

Fixes some duplicate route names in routes/api.php for the Api\AssetFilesController:

  • api.assets.files was used for both list and store
  • api.assets.file was used for both show and destroy

This led to artisan route:cache and artisan optimize to fail (also see #11595 which fixed this for web routes).

This PR also introduces a new OptimizeTest to ensure that artisan optimize runs successfully.

Fixes #10224

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Tests\Feature\Console\OptimizeTest

@dbakan dbakan requested a review from snipe as a code owner July 12, 2024 20:12
Copy link

welcome bot commented Jul 12, 2024

💖 Thanks for this pull request! 💖

We use semantic commit messages to streamline the release process and easily generate changelogs between versions. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix if it doesn't have one already.

Examples of commit messages with semantic prefixes:

  • Fixed #<issue number>: don't overwrite prevent_default if default wasn't prevented
  • Added #<issue number>: add checkout functionality to assets
  • Improved Asset Checkout: use new notification method for checkout

Things that will help get your PR across the finish line:

  • Document any user-facing changes you've made.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@dbakan dbakan changed the title Fix route names for optimize command Fixed route names for optimize command Jul 12, 2024
Copy link

what-the-diff bot commented Jul 12, 2024

PR Summary

  • Implementation of a New API Route
    A fresh API path has been incorporated to enhance our system's accessibility and functionality. This new route, named ‘/api/assets/files’, is now available and its name has been updated in the routes/api.php file.

  • Updated API Endpoints in Testing
    Adjustments have been done to the API endpoints in our test files, more precisely, in tests/Feature/Assets/Api/AssetFilesTest.php. They now utilize the newly named route, which harmonizes our testing with the latest changes and assures the continuation of robust, efficient testing of our API routes.

  • Addition of a New Test Case
    The advent of a new file, tests/Feature/Console/OptimizeTest.php, introduces a test case for the optimize command to the system. This is a step towards increased system resilience by ensuring this command operates correctly and meets our operational and performance standards.

@dbakan dbakan changed the title Fixed route names for optimize command Fixed #10224: fix route names for optimize command Jul 12, 2024
@snipe
Copy link
Owner

snipe commented Jul 13, 2024

Thanks! This PR seems fine - we don't use the route:optimize console command, but it shouldn't hurt anything. Can you please re-target to the develop branch though, per the developer docs?

@dbakan dbakan changed the base branch from master to develop July 13, 2024 14:29
@dbakan
Copy link
Contributor Author

dbakan commented Jul 13, 2024

Thanks! I changed the target branch to develop. Sorry I missed that.

@snipe
Copy link
Owner

snipe commented Jul 13, 2024

Thanks! Looks like you might need to rebase. There's a bunch of stuff in there outside of this PR's changes files/commits.

@dbakan dbakan marked this pull request as draft July 13, 2024 14:32
@dbakan dbakan force-pushed the fix/optimize-command branch from 63a33c1 to a175b6a Compare July 13, 2024 14:36
@dbakan dbakan marked this pull request as ready for review July 13, 2024 14:37
@dbakan
Copy link
Contributor Author

dbakan commented Jul 13, 2024

Yeah, you're right. I hope it's fixed now.

@snipe snipe merged commit be43173 into snipe:develop Jul 13, 2024
9 checks passed
Copy link

welcome bot commented Jul 13, 2024

Congrats on merging your first pull request! 🎉🎉🎉

@dbakan dbakan deleted the fix/optimize-command branch July 23, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

artisan route:cache Unable to prepare route [api/v1] for serialization. Uses Closure.
2 participants