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

General optimizations around Persisters for simplicity and sanity of us all. #1246

Merged
merged 5 commits into from
Jan 13, 2015

Conversation

guilhermeblanco
Copy link
Member

No description provided.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3487

We use Jira to track the state of pull requests and the versions they got
included in.

foreach ($diff as $element) {
$this->conn->executeUpdate($sql, $this->getInsertRowSQLParameters($coll, $element));
}
throw new \BadMethodCallException("Deleting elements is not supported by this CollectionPersister.");
Copy link
Member

Choose a reason for hiding this comment

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

Make the method abstract instead?

@guilhermeblanco guilhermeblanco changed the title [WIP] Moved delete() and update() to proper locations. General optimizations around Persisters for simplicity and sanity of us all. Jan 13, 2015
@Ocramius Ocramius self-assigned this Jan 13, 2015
@Ocramius Ocramius merged commit bc268da into master Jan 13, 2015
Ocramius added a commit that referenced this pull request Jan 13, 2015
Ocramius added a commit that referenced this pull request Jan 13, 2015
Ocramius added a commit that referenced this pull request Jan 13, 2015
Ocramius added a commit that referenced this pull request Jan 13, 2015
Ocramius added a commit that referenced this pull request Jan 13, 2015
Ocramius added a commit that referenced this pull request Jan 13, 2015
Ocramius added a commit that referenced this pull request Jan 13, 2015
@Ocramius Ocramius deleted the optimize-persisters branch January 13, 2015 14:06
@Ocramius
Copy link
Member

@guilhermeblanco merged, thanks!

master: 9c3cb57

* Gets the SQL parameters for the corresponding SQL statement to delete the given
* element from the given collection.
*
* @internal Order of the parameters must be the same as the order of the columns in getDeleteRowSql.
Copy link
Member

Choose a reason for hiding this comment

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

This should not be documented this way. It is a usage of @internal which does not match the phpdoc standards. @internal is about marking a method as being internal, not about providing advanced documentation which should be hidden while other parts are visible

Copy link
Member

Choose a reason for hiding this comment

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

I'll have another run at all @internal docblocks later, as it is indeed getting annoying in IDEs :-)

Copy link
Member

Choose a reason for hiding this comment

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

Btw, this cleanup has been done in DBAL a while ago

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