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

Locale Repository #702

Merged
merged 3 commits into from Jun 6, 2019

Conversation

Projects
None yet
3 participants
@xfudox
Copy link

commented May 16, 2019

Recently I had the need to handle locales as data to be saved along models (e.g.: a list of spoken languages), so I made a simple vue remote select component with its own api endpoint, request, controller and repository.

silvio-gratani added some commits May 16, 2019

added:
- vue select component
- api route
- api controller
- request
- repository interface
- eloquent repository
- repository binding
- transformer
fix
@nWidart

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Hi,
Thank you for your contribution! This looks good. I'm wondering what the form request is for, it doesn't look like it can persist something?

@xfudox

This comment has been minimized.

Copy link
Author

commented May 20, 2019

I've been busy these day, so as soon as i can i'll fix the branch to pass the tests.

To answer your question i created a custom request just to validate the data sent to the LocaleRepository, probably extending it from the FormRequest is not necessary, but i can't understand what you mean when you say about the possibility to have it persist something, maybe i'm missing something from the whole picture.

@xfudox

This comment has been minimized.

Copy link
Author

commented May 22, 2019

Ok, i gave a look to the integretion tests errors: it is a test from the Asgard Media module, MaxFolderSizeRuleTest::it_validates_max_folder_size_is_valid

How can this test fail? my merge didn't affect (I think/hope) anything related to this.
Maybe i'm missing something.

@nWidart

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Hello,

You can ignore that test yeah, I need to disable it.

By persisting I mean writing on disk.
That's why I assumed when I saw a form request. What data is sent to the repository that needs to be validated?

@xfudox

This comment has been minimized.

Copy link
Author

commented May 23, 2019

I made a request just to validate the data for the research, not to validate data to be saved.

I consider the api endpoint used by the vue component an "unsafe entrypoint", so i made along it a related request to validate the data given for the research and ensure my eloquent repository logic receive correct data to work on.

@xfudox

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

Bumping this up

Any news?

@nWidart

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Thanks for the bump, sorry 🤗

Looks good, thank you! 👍

@nWidart nWidart merged commit 265d5cf into AsgardCms:master Jun 6, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@xfudox

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

Thanks @nWidart 👍

mikemand added a commit to mikemand/Platform that referenced this pull request Jun 13, 2019

Fix mismatched implementation from PR AsgardCms#702
Signed-off-by: Micheal Mand <micheal@kmdwebdesigns.com>

nWidart added a commit that referenced this pull request Jun 14, 2019

Merge pull request #706 from mikemand/hotfix/Locale-Implementation-Mi…
…smatch

Fix mismatched implementation from PR #702
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.