Skip to content
This repository has been archived by the owner on Jul 28, 2022. It is now read-only.

Improve fixtures 3 #636

Merged
merged 1 commit into from
Jul 27, 2020

Conversation

wbloszyk
Copy link
Member

No description provided.

@wbloszyk wbloszyk force-pushed the improve_fixtures_3 branch 2 times, most recently from 8588bf9 to 253fc73 Compare July 20, 2020 15:47
@wbloszyk wbloszyk marked this pull request as ready for review July 20, 2020 15:48
@jordisala1991 jordisala1991 requested a review from a team July 21, 2020 11:42
@jordisala1991 jordisala1991 requested a review from a team July 24, 2020 06:21
@wbloszyk wbloszyk force-pushed the improve_fixtures_3 branch 2 times, most recently from 69efdc6 to 440b0d1 Compare July 26, 2020 19:11
@wbloszyk
Copy link
Member Author

@core23 @VincentLanglet @phansys Can you check it? It is RTM and require for next PRs.


$gallery->addGalleryHasMedias($galleryHasMedia);
}

protected function createMedia($file, string $name, string $autor, Category $category, bool $enabled = true, string $copyright = 'CC BY-NC-SA 4.0')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this method be private?

Suggested change
protected function createMedia($file, string $name, string $autor, Category $category, bool $enabled = true, string $copyright = 'CC BY-NC-SA 4.0')
protected function createMedia(string $file, string $name, string $autor, Category $category, bool $enabled = true, string $copyright = 'CC BY-NC-SA 4.0')

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$file can be string or File like setBinaryContext.

IMHO we should keep Api open as much as it is possible in Demo. This will allow user to people to test it and understand how sonata work.

Copy link
Member

@phansys phansys Jul 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok then. Could you please add a docblock for this argument?

@wbloszyk
Copy link
Member Author

@phansys @jordisala1991 Can we merge it?

@jordisala1991
Copy link
Member

IMO yes, @phansys wdyt?

@phansys
Copy link
Member

phansys commented Jul 27, 2020

@phansys @jordisala1991 Can we merge it?

I just leave a small suggestion about an argument's docblock. IMO, it's RTM.

@VincentLanglet
Copy link
Member

The phpdoc could be indeed great before merging this.

@wbloszyk
Copy link
Member Author

I need this PR to webpack. I will work on dobclock, update API, fix file structure in next PRs too. So lets merge it now.

BTW. I working on sandbox 3.0

@VincentLanglet
Copy link
Member

This is an open source project, every body can use it, working on it.
Every PR, every commit should be self-sufficient.

We're just asking this change.

/**
 * @param string|File $file
 */ 
protected function createMedia($file, string $name, string $autor, Category $category, bool $enabled = true, string $copyright = 'CC BY-NC-SA 4.0')

@wbloszyk
Copy link
Member Author

This is an open source project, every body can use it, working on it.
Every PR, every commit should be self-sufficient.

We're just asking this change.

/**
 * @param string|File $file
 */ 
protected function createMedia($file, string $name, string $autor, Category $category, bool $enabled = true, string $copyright = 'CC BY-NC-SA 4.0')
/**
     * @param string|File $file File pathname or `Symfony\Component\HttpFoundation\File` object
     *
     * @return Media
     */
    protected function createMedia(

@VincentLanglet VincentLanglet merged commit 6726ea7 into sonata-project:master Jul 27, 2020
@wbloszyk wbloszyk deleted the improve_fixtures_3 branch July 27, 2020 13:52
VincentLanglet pushed a commit that referenced this pull request Jul 27, 2020
* improve fixtures

* Add static code analysis through GitHub actions

* Improve fixtures 2

* Add initial Docker configuration

* Add dependabot to keep dependencies up to date

* Improve fixtures 3 (#636)

Co-authored-by: Javier Spagnoletti <[email protected]>
Co-authored-by: Jordi <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants