Fix: Compare Action route

[BurningDog]

Subject

This fixes the issue where a revision could not be viewing via the CompareAction route.

I am targeting the 1.x branch because it's a bug fix which is backwards compatible.

Closes #405.

Changelog

### Fixed

- The CompareAction route is now working.

[VincentLanglet]

Array are supposed to be supported

EntityAuditBundle/src/AuditReader.php

Line 617 in e38bb57

* @param int|string|array $id

and find support array too

EntityAuditBundle/src/AuditReader.php

Line 220 in e38bb57

public function find($className, $id, $revision, array $options = [])

I would say we try to support something like 3,4,5.

Do it mean this case is broken

EntityAuditBundle/src/AuditReader.php

Line 235 in e38bb57

if (\is_array($id) && \count($id) > 0) {

?

@wbloszyk @phansys you're using this bundle, do you pass array/multiple ids some times ?

[BurningDog]

If we pass $ids as an array to the find() function, then inside

EntityAuditBundle/src/AuditReader.php

Lines 235 to 237 in e38bb57

	if (\is_array($id) && \count($id) > 0) {
	$idKeys = array_keys($id);
	$columnName = $idKeys[0];

the values are:

• $idKeys = [0 => 0]
• $columnName = 0

...which leads to

EntityAuditBundle/src/AuditReader.php

Line 246 in e38bb57

$whereSQL .= ' AND e.'.$columnName.' = ?';

being

$whereSQL = "e.rev <= ? AND e.0 = ?" which leads to the sql error.

While I think the type of $id at

EntityAuditBundle/src/AuditReader.php

Line 220 in e38bb57

public function find($className, $id, $revision, array $options = [])

is correct, I think for the diff() function at

EntityAuditBundle/src/AuditReader.php

Line 617 in e38bb57

* @param int|string|array $id

it should be * @param int|string $id - because I think a diff is on a single object, by definition.

In that case, the new value becomes:

$whereSQL = "e.rev <= ? AND e.id = ?" which is correct.

[BurningDog]

...I've added this change into the PR, as CompareAction is the only place which calls the diff() function in the AuditReader.

[phansys]

@wbloszyk @phansys you're using this bundle, do you pass array/multiple ids some times ?

I'm not using composite ids with this bundle.
This issue was reported before. See #288, #331 #389.

[FlintCIBot]

Lint errors were found. A patch is also available.

Please see the report: https://flintci.io/repositories/8593/analyses/63517

This comment was posted by FlintCI. It can be disabled in the repository settings.

[VincentLanglet]

@BurningDog Can you do the same for others actions like in PR https://github.com/sonata-project/EntityAuditBundle/pull/331/files ?

[BurningDog]

@VincentLanglet ok, done - I searched for all calls to the find() and findRevisions() functions to ensure that the passed $id is either an int or a string.

[phansys]

Are these changes disallowing the possibility of using entities with composite ids?
If yes, I think we must add validations to prevent these entities to be marked as auditable from the beginning.

[VincentLanglet]

Are these changes disallowing the possibility of using entities with composite ids?

If yes, I think we must add validations to prevent these entities to be marked as auditable from the beginning.

Composite ids can be used if you call directly the method of the AuditReader, but cannot be used with these actions.

I dont think it's specifically new since you cannot pass composite ids in a url. Can u ?

Imho if something should be added it would be better to create an issue about, in order to not delay this bugfix

[phansys]

I dont think it's specifically new since you cannot pass composite ids in a url. Can u ?

Yes, by instance you can use this approach for the ORM manager: "/app/entity/1~42/show".

Imho if something should be added it would be better to create an issue about, in order to not delay this bugfix

I agree.