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

Add ArrayCollection map() method capability to access inner array key #221

Open
wants to merge 1 commit into
base: 2.0.x
Choose a base branch
from

Conversation

didix16
Copy link

@didix16 didix16 commented Nov 15, 2019

This enhancement gives to map() Closure the capability to access also the keys of the inner array holding the elements. Until now, we were able to access just the element on map Closure but not the key. So I propose to add the ability to access also the key for do some statements inside the Closure with key related.

I have the need to add it since in my project I must get the asociative key to take a decision in function of the current key element and I thought that may be usefull for everyone in some other projects to access the key of current processing element

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Please adjust commit message (to explain what is being done), PR title and tests (required)

lib/Doctrine/Common/Collections/ArrayCollection.php Outdated Show resolved Hide resolved
@someniatko
Copy link
Contributor

someniatko commented Nov 15, 2019

I think you should also update PHPDoc for the Collection interface. Usually doctrine collections are used in context of Doctrine ORM, and it may replace entity's collections with e.g. PersistentCollection. But the user will anyway expect the same behavior for it like for ArrayCollection. Therefore I don't see any sense limiting mapping keys functionality to ArrayCollection only.

@alcaeus
Copy link
Member

alcaeus commented Nov 15, 2019

I don't think we should update the documentation for the Collection interface: not all collection will implement this functionality that way.

@someniatko
Copy link
Contributor

someniatko commented Nov 15, 2019

I always thought that ArrayCollection is just an implementation of original Collection interface, and in general it shouldn't add more functionality than declared by the original interface. Because e.g. i have

/** @Entity */
class User
{
    /** @OneToMany(...) */
    private $comments;

    public function __construct()
    {
        $this->comments = new ArrayCollection();
    }

    public function getCommentStats(): Collection
    {
        return $this->comments->map(function ($value, $key) {
             return new CommentStatistics($key, $value);
        }); 
    }
}

I expect that getCommentStats() behaves exactly the same if i've just created new User, and if I fetched this User from a repository. In the latter case the underlying Collection implementation for $comments is no longer ArrayCollection!

@Ocramius
Copy link
Member

There's a BC problem here if we move it to Collection. There's a type problem if we don't move it to Collection

Specifically, the signature is currently something like Collection#map(callable (TVal=) : V) : Collection<TKey, V>.

If we change it to Collection#map(callable (TVal=, TKey=) : V) : Collection<TKey, V>, then consumers are still free to ignore the second callable parameter (undefined behavior).

If we don't change it, then Collection#map(callable (TVal=) : V) : Collection<TKey, V> is explicitly incompatible with calling #map with callable (Foo, Bar) : V as parameter, since this callable is of a stricter (type) than what Collection#map() allows.

I don't have a solution for now, but I hope this explains the problem.

@someniatko
Copy link
Contributor

There's a BC problem

This PR targets master so it shouldn't be a problem afaik.

@Ocramius Ocramius added this to the 2.0.0 milestone Nov 15, 2019
@didix16 didix16 changed the title Update ArrayCollection.php Add ArrayCollection map() method capability to access inner array key Nov 15, 2019
Copy link
Author

@didix16 didix16 left a comment

Choose a reason for hiding this comment

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

Fixed PHPDoc misstype

@greg0ire
Copy link
Member

greg0ire commented Nov 15, 2019

Please improve your commit message according to the contributing guide.

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

  1. git rebase -i origin/master, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

@didix16
Copy link
Author

didix16 commented Nov 16, 2019

Please improve your commit message according to the contributing guide.

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

  1. git rebase -i origin/master, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

Oh sorry. This is my first PR. I never do it before ( Im very newbie ). I just do it the modifications at Github, not in my local, so I don't know if I can do the rebase at the web page.

What should I do now? I have to clone the repository then?

P.S: For future commits and PR I'll keep in my mind the procedure to make a proper PR.

Thanks in advance

@greg0ire
Copy link
Member

greg0ire commented Nov 16, 2019

I can totally do it for you if you don't feel comfortable jumping through the following hoops:

# Clone the repo
git clone git@github.com:doctrine/collections.git

# Enter it
cd collections

# Add your fork as a remote
git remote add fork git@github.com:didix16/collections.git

# Fetch your patch-1 branch, the overkill way
git fetch --all

# Checkout your branch
git checkout patch-1

# Follow my previous instructions for squashing
git rebase -i origin/master

# Rework your commit message
git commit --amend

# Finally, use the force
git push --force

@didix16
Copy link
Author

didix16 commented Nov 16, 2019

I can totally do it for you if you don't feel comfortable jumping through the following hoops:

# Clone the repo
git clone git@github.com:doctrine/collections.git

# Enter it
cd collections

# Add your fork as a remote
git remote add fork git@github.com:didix16/collections.git

# Fetch your patch-1 branch, the overkill way
git fetch --all

# Checkout your branch
git checkout patch-1

# Follow my previous instructions for squashing
git rebase -i origin/master

# Rework your commit message
git commit --amend

# Finally, use the force
git push --force

Ok, thank you so much. I'll do it right now and see what happens :) 👍

This enhancement gives to map() Closure the capability to access also the keys of the inner
array holding the elements. Until now, we were able to access just the element on map Closure
but not the key. So I propose to add the ability to access also the key for do some statements
inside the Closure with key related.

I have the need to add it since in my project I must get the asociative key to take a decision
in function of the current key element and I thought that may be usefull for everyone in some
other projects to access the key of current processing element.
greg0ire
greg0ire previously approved these changes Nov 16, 2019
@someniatko

This comment has been minimized.

@greg0ire

This comment has been minimized.

@VincentLanglet
Copy link
Contributor

If we add keys to the map method, maybe the key should be the first param to add consistency with others methods.
See #251

Base automatically changed from master to 2.0.x January 19, 2021 07:24
@greg0ire
Copy link
Member

This seems stalled, I'm removing it from the 2.0.0 milestone.

@greg0ire greg0ire removed this from the 2.0.0 milestone Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants