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

EZP-21324: Fixed missing references to copied image attributes #792

Closed
wants to merge 23 commits into from

Conversation

@bdunogier
Copy link
Member

commented Oct 16, 2013

See http://jira.ez.no/browse/EZP-21324

Replaced by #825

This PR fixes a use-case where image files from content A might be deleted when content B is deleted.

Use-case

  • create a content A with an image attribute
  • edit content A, but don't change the image.
    • Version 1 references the file. An entry exists in ezimagefile.
    • Version 2 will reference the same image than version 1. A (duplicate) entry exists in ezimagefile.
  • copy content A to content B
    • Version 1 references the same image than A version 1. There is no reference from this attribute to the image in ezimagefile
    • Version 2 references a copy (actually a hard link, but it doesn't matter) of the image from A version 2. An entry exists in ezimagefile.

The consequence is that when B version 1 gets deleted (either manually, or because the version limit gets reached, or when B is deleted, the image used by A (both versions) will get deleted.

Fix

The PR fixes:

  • what is done when copying (by adding references to the image used in v1 to ezimagefile)
  • what is done when deleting (usage of the image is checked for any attribute, using ezimagefile, since we have proper references)
  • existing data, by providing an update script, update/common/scripts/5.3/recreateimagesreferences.php. The script goes through all ezimage attributes, and adds missing references to ezimagefile.

Changes

Removal of aliases has been totally refactored, and should work fine with the way we store data now.
purgeAliases(), used by eZCache, has been refactored to use the same code (with tests).

TODO

  • Fix the removeAliases() code (used when a specific version is removed)
  • Fix the postgresql error in eZImageFile::fetchListByFilePath() (group by error)
  • Fix the removeAllAliases() code (used when a complete content object is removed)
  • Fix the remaining reference to the deleted attribute left in the database when deleting a copy of an image content
  • Improve tests to check that no ezimagefile records are left behind
  • Add translated data to tests
  • Rebase, keeping tests in separate commits for backport
bdunogier added 22 commits Oct 15, 2013
EZP-21324: Fixed missing references to copied image attributes
- eZImageType::postStore() adds references from ezimage attributes
- An update script (update/common/5.3/restoreimageslinks.php will add missing
  relations for existing data
EZP-21324: Fixed regression when deleting draft
Also changed removeAlias code. Should now check correctly:
- only remove ezimagealias entries when they're not used anymore
  (handles duplicates if any)
- only removed files when not used anymore.

This means that we accept that when a copy of a content with an
image is made, we accept that archived versions in the copy will
reference the same file than archive versions in the copy source.
EZP-21324: changed removeAlias() prototype
Doesn't accept an eZContentObjectAttribute argument anymore.
The method is always invoked on the instance it must be modified.
EZP-21324: Fixed postgres error
fetchListByFilePath now uses asObject=false, since it doesn't
really make sense to return incomplete objects (due to the duplicates
inserted in ezimagefile)
EZP-21324: fixed removeAllAliases()
We just don't use it anymore. removeAliases() does the job.
The only difference is that removeAliases() will set the
attribute's XML to an empty value, while removeAllAliases()
won't, meaning that the change will create a light overhead
on the database.
EZP-21324: tests (failing)
testRemoveCopy also checks that the deleted attributes are no
longer referenced by any ezimagefile record
EZP-21324: Prevented duplicates in ezimagefile
`eZImageType::postStore()` won't add duplicates
to ezimagefile anymore.
EZP-21324: Added test case to removeCopy
Now tests that all alias files are effectively
removed from ezimagefile.

Current fails on one of the V1 aliases.
EZP-21324: Added a test case (fails)
Tests the case where the published version of an image
has more aliases than the archived versions: during copy(),
references to those aliases are created by eZImageType::postStore(),
but are copied to new references instead of being removed,
since the attribute is not the image owner.

These ezimagefile entries are orphan ones, and aren't removed
when the object is deleted.
@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2013

Note: the failure above shouldn't be related to this PR (StaleCache thing).

$aliasList = $this->aliasList();
$alternativeText = false;
if ( $aliasFile == '' )
throw new InvalidArgumentException( "Expecting image file path" );

This comment has been minimized.

Copy link
@lolautruche

lolautruche Oct 21, 2013

Contributor

So we use exceptions now in legacy ? 😉

* Remove the attribute $id by using the datatype.
* @param int $id
* @param int $currentVersion Version number to remove the attribute for. If null, all versions are removed.
*/

This comment has been minimized.

Copy link
@lolautruche

lolautruche Oct 21, 2013

Contributor

Does it belong to this PR ?

null,
null, // @todo Implement batch fetch
false
);

This comment has been minimized.

Copy link
@lolautruche

lolautruche Oct 21, 2013

Contributor

Warning, potential memory leak since you can have a lot of results here, depending on the installation.
I guess the @todo is here for that 😃 .

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2013

Wow, this is speleology !
Code looks OK. And assuming your tests cover it all: +1

Review ping @andrerom @dpobel @yannickroger @pspanja

@yannickroger

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2013

After todos are completed +1
Congrats for sorting all that code out 👍

@andrerom

This comment has been minimized.

Copy link
Member

commented Oct 22, 2013

+1

continue;
// Creates ezimagefile entries
foreach ( $doc->xpath( "//*/@url" ) as $url )

This comment has been minimized.

Copy link
@patrickallaert

patrickallaert Oct 28, 2013

Contributor

"//@url" might be more efficient to handle?

return;
// Creates ezimagefile entries
foreach ( $doc->xpath( "//*/@url" ) as $url )

This comment has been minimized.

Copy link
@patrickallaert

patrickallaert Oct 28, 2013

Contributor

"//@url" might be more efficient to handle?

This comment has been minimized.

Copy link
@bdunogier

bdunogier Oct 28, 2013

Author Member

Well, it comes from your patch, so...

This comment has been minimized.

Copy link
@patrickallaert

patrickallaert Oct 28, 2013

Contributor

I missed reviewing my own work ;)

@@ -679,150 +682,115 @@ static function removeAllAliases( $contentObjectAttribute )
*
* @param eZContentObjectAttribute $contentObjectAttribute

This comment has been minimized.

Copy link
@patrickallaert

patrickallaert Oct 28, 2013

Contributor

To be removed.

@pspanja

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2013

Looks good so far, todos aside: +1

@bdunogier bdunogier referenced this pull request Nov 5, 2013
1 of 1 task complete
bdunogier added a commit that referenced this pull request Nov 6, 2013
EZP-21324: Added tests
Taken from #792
bdunogier added a commit that referenced this pull request Nov 7, 2013
EZP-21324: Added tests
Taken from #792
@bdunogier

This comment has been minimized.

Copy link
Member Author

commented Nov 7, 2013

Replaced by #825.

@bdunogier bdunogier closed this Nov 7, 2013

@andrerom andrerom deleted the fix/EZP-21324-copy_old_version_images branch Nov 7, 2013

bdunogier added a commit that referenced this pull request Nov 8, 2013
bdunogier added a commit that referenced this pull request Nov 8, 2013

@bdunogier bdunogier restored the fix/EZP-21324-copy_old_version_images branch Mar 22, 2014

@bdunogier bdunogier referenced this pull request Apr 1, 2014
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.