Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

API Allow the modification of extraFields on a many_many relation #754

Closed
wants to merge 2 commits into from

4 participants

Howard Grigg Don't Add Me To Your Organization a.k.a The Travis Bot Sam Minnée Ingo Schommer
Howard Grigg

Before you could not modify the extraFields on a many_many relation without deleting and recreating the relation. This meant you would lose all saved extraFields and just have the one you added.

This is fixed with a new function modify().

Let me know what you think.

Howard Grigg howardgrigg API Allow modifying the extraFields on a many_many
Before you could not modify the extraFields on a many_many relation
without deleting and recreating the relation. This meant you would lose
all saved extraFields and just have the one you added. This is fixed
with modify().
361b865
Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request fails (merged 361b865 into a71077c).

Sam Minnée
Owner

I don't want to bolt on a new API for this without thinking a bit more carefully about what we're doing.

Right now the way you write an object is to call the write method on the object itself:

$list = $something->SomeManyManyRelation();
$object = $list->First();
$object->SomeField = "value";
$object->SomeManyManyExtraField = "value";
$object->write();

This is fine for regular fields but breaks down with many_many_extraFields:

$list = $something->SomeManyManyRelation();
$object = $list->First();
$object->SomeManyManyExtraField = "value";
$object->write(); // nothing happens

Rather than have a special way of dealing with many_many_extraFields, I'd prefer to have a generalised API that classes like GridField can "just use" without worrying about the details. This would work:

$list = $something->SomeManyManyRelation();
$object = $list->First();
$object->SomeManyManyExtraField = "value";
$list->update($object); // calls $object->write() as well as updating the many-many list

This could be extended to things other than many-many list:

$openIssues = Issue::get()->filter(array("Status" => "Open"));
// This new API would define what happens when an record is added to this list
$openIssues->setFieldUpdatingHints(array("Status" => "Open"));

$i = new Issue;
$i->Title = "Something";
$openIssues->update($i); // Status would be set to "Open" as well as $i being written to the database.

If used consistently, the DataObjects wouldn't need to worry about their own persistence nearly as much, which would make it easier to swap in different persistence back-ends.

It's in this context that I think we should solve the problem that Howard has identified. As I've spec'ed it out, it would mean:

  • Create DataList::update($object) as a new way of writing $object.
  • Remove direct $object->write() calls in GridField, etc, in favour of $list->update($object).
  • Override ManyManyList::update() in a manner similar to Howard's suggestions.
  • Further down the line: introduce setFieldUpdatingHints(), or even look at automatically inferring this from filter() commands.

However, we'd need to agree on the API first.

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request passes (merged 9efa246 into a71077c).

Ingo Schommer
Owner

The immediate bug (removes extrafields on add) has been fixed in #810.
Sam's ideas are an ongoing discussion on the mailinglist,
and diverge quite a bit from the code changes in this pull request - so closing off.
Thanks for diving into this anyway, Howard! :)

Ingo Schommer chillu closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 30, 2012
  1. Howard Grigg

    API Allow modifying the extraFields on a many_many

    howardgrigg authored
    Before you could not modify the extraFields on a many_many relation
    without deleting and recreating the relation. This meant you would lose
    all saved extraFields and just have the one you added. This is fixed
    with modify().
  2. Howard Grigg
This page is out of date. Refresh to see the latest.
Showing with 47 additions and 0 deletions.
  1. +35 −0 model/ManyManyList.php
  2. +12 −0 tests/model/DataObjectTest.php
35 model/ManyManyList.php
View
@@ -96,6 +96,41 @@ function add($item, $extraFields = null) {
}
/**
+ * Modify an item in this many_many relationship
+ * Does so by updating an entry in the joinTable.
+ * @param $extraFields A map of additional columns to insert into the joinTable
+ */
+ function modify($item, $extraFields = null) {
+ if(is_numeric($item)) $itemID = $item;
+ else if($item instanceof $this->dataClass) $itemID = $item->ID;
+ else throw new InvalidArgumentException("ManyManyList::modify() expecting a $this->dataClass object, or ID value", E_USER_ERROR);
+
+ // Validate foreignID
+ if(!$this->foreignID) {
+ throw new Exception("ManyManyList::modify() can't be called until a foreign ID is set", E_USER_WARNING);
+ }
+
+ // Check relation already exists
+ if(!$this->find($this->localKey, $itemID)){
+ throw new Exception("ManyManyList::modify() can't be called unless the relation is already set", E_USER_WARNING);
+ }
+
+ // Modify entry/entries
+ foreach((array)$this->foreignID as $foreignID) {
+ $manipulation = array();
+ $manipulation[$this->joinTable]['command'] = 'update';
+
+ if($extraFields) foreach($extraFields as $k => $v) {
+ $manipulation[$this->joinTable]['fields'][$k] = "'" . Convert::raw2sql($v) . "'";
+ }
+
+ $manipulation[$this->joinTable]['where'] = "\"$this->localKey\" = $itemID AND \"$this->foreignKey\" = $foreignID";
+
+ DB::manipulate($manipulation);
+ }
+ }
+
+ /**
* Remove the given item from this list.
* Note that for a ManyManyList, the item is never actually deleted, only the join table is affected
* @param $itemID The ID of the item to remove.
12 tests/model/DataObjectTest.php
View
@@ -770,6 +770,18 @@ function testManyManyExtraFields() {
$this->assertEquals('Sam', $player->FirstName);
$this->assertEquals("Prop", $player->Position);
+ // Sam deserves instead to be on the wing
+ $newTeam->Players()->modify($newPlayer, array("Position" => "Winger"));
+
+ // Requery and uncache everything
+ $newTeam->flushCache();
+ $newTeam = DataObject::get_by_id('DataObjectTest_Team', $newTeamID);
+
+ // Check that the modified Position many_many_extraField is extracted.
+ $player = $newTeam->Players()->First();
+ $this->assertEquals('Sam', $player->FirstName);
+ $this->assertEquals("Winger", $player->Position);
+
// Check that ordering a many-many relation by an aggregate column doesn't fail
$player = $this->objFromFixture('DataObjectTest_Player', 'player2');
$player->Teams("", "count(DISTINCT \"DataObjectTest_Team_Players\".\"DataObjectTest_PlayerID\") DESC");
Something went wrong with that request. Please try again.