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

[FEATURE] Add CollectionViewHelper #578

Merged
merged 1 commit into from
Aug 13, 2014

Conversation

dimaip
Copy link
Contributor

@dimaip dimaip commented May 27, 2014

It's the first time I try to commit something on Github, so help me out if I'm doing something really wrong.

I didn't know to what category to add this VH to, so feel free to relocate it.

@bjo3rnf
Copy link
Contributor

bjo3rnf commented May 27, 2014

Hi @dimaip,

thanks for contributing. There's some minor CGL cosmetics to do though. I'll add some line comments. For the record this PR closes #577.

Cheers
Björn

@cedricziel
Copy link
Member

Thank you very much! I hope you dont mind the comments :)

Cheers,
Cedric

@dimaip
Copy link
Contributor Author

dimaip commented May 29, 2014

One more question: when I'm working at the file locally, do I have to checkout into separate branch for every edit that I make and then merge, or can I just work on the development branch locally?

edit: OK, I got it, I didn't need to merge with development locally before merge, i should have used git push origin my_local_fix_branch:development

edit2: now I get it... I should have created a separate branch not just locally, but also in my github forked repository... And also I've just discovered there's a way to add and edit files straight from github's web interface. For small edits it may be more convenient.


/**
* @var \TYPO3\CMS\Core\Collection\RecordCollectionRepository
* @inject
Copy link
Member

Choose a reason for hiding this comment

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

Please use an injection method - it's more efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, didn't know that! I was curious of why you still use those clumsy injection methods...
I'll fix it this evening.

@bjo3rnf
Copy link
Contributor

bjo3rnf commented Jul 16, 2014

Hi @dimaip,

are you still into this PR? @NamelessCoder explained his objections against the findAll() method and if this is not a showstopper for you I'd like to ask you to implement his suggestions. In case of any questions feel free to ask.

Cheers
Björn

@dimaip
Copy link
Contributor Author

dimaip commented Jul 16, 2014

I'll try to find time tomorrow.
Sorry for delay, got buried by work.

Sent from mobile.
On Jul 16, 2014 7:44 PM, "Björn Fromme" notifications@github.com wrote:

Hi @dimaip https://github.com/dimaip,

are you still into this PR? @NamelessCoder
https://github.com/NamelessCoder explained his objections against the
findAll() method and if this is not a showstopper for you I'd like to ask
you to implement his suggestions. In case of any questions feel free to ask.

Cheers
Björn


Reply to this email directly or view it on GitHub
#578 (comment).

@dimaip
Copy link
Contributor Author

dimaip commented Jul 31, 2014

Have a look, guys, I've tried to update the code according to @NamelessCoder's suggestions. Sorry for being so slow :(

@NamelessCoder
Copy link
Member

Almost there - just a bit of cleanup to do and one error prevention measure:

Before calling a method on the value returned from the ResourceRepository the value must be checked. If for example an invalid reference is passed, the current code would cause a fatal PHP error (method called on non-object). To safeguard, early-return NULL if $collection is empty.

And to clean up the git commit history:

Do git reset HEAD^1 until the command line reports that you are on the commit [FEATURE] Add CollectionViewHelper and then: git commit --amend and finally git push --force. You can do this after you make the changes in code. When you are done and if done correctly, you should see a single commit in the "Commits" tab above.

@dimaip
Copy link
Contributor Author

dimaip commented Aug 12, 2014

Sorry for being turtle-slow :(
I think I've done what you asked for.

@NamelessCoder
Copy link
Member

I didn't notice until now, but the class is using the old style class name and should be using namespaces, e.g. namespace FluidTYPO3\Vhs\ViewHelpers\Resource (note: this moves VH to v:resources.collection) and should import by use all classes currently written as full class names.

Code behaviour is perfect now :)

Use RecordCollection API to retrieve RecordCollection by uid.
@dimaip
Copy link
Contributor Author

dimaip commented Aug 13, 2014

Like this?

@NamelessCoder
Copy link
Member

Exactly like that :) thanks Dmitri :)

NamelessCoder added a commit that referenced this pull request Aug 13, 2014
[FEATURE] Add CollectionViewHelper
@NamelessCoder NamelessCoder merged commit 00a9eb1 into FluidTYPO3:development Aug 13, 2014
@dimaip
Copy link
Contributor Author

dimaip commented Aug 13, 2014

Thanks for your patience, Claus. I'm glad it worked out, after all!

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.

4 participants