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

fix: fix copy of copied classes #3857

Closed
wants to merge 5 commits into from

Conversation

tikhanovichA
Copy link
Contributor

https://oat-sa.atlassian.net/browse/ADF-1545

We need to fix the possibility of copying already copied classes.

How to test:

  1. Go to items and create a new folder/class
  2. Copy this class
  3. Try to copy the folder you got in step 2

Copy link
Contributor

@uncleempty uncleempty left a comment

Choose a reason for hiding this comment

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

In general it's ok.
Just one question:
maybe it's better to create $copyIdentifier in the doCopy method instead of creating it higher and providing to each call of this method? Because as far as I can see all the needed args are also available inside. And this will make changes way less intrusive.

models/classes/resources/Service/ClassCopier.php Outdated Show resolved Hide resolved
models/classes/resources/Service/ClassCopier.php Outdated Show resolved Hide resolved
@@ -96,9 +96,18 @@ public function copy(
private function doCopy(
core_kernel_classes_Class $class,
core_kernel_classes_Class $destinationClass,
bool $keepOriginalPermission = true
bool $keepOriginalPermission = true,
string $copyIdentifier = '',
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to use null as default instead of empty string 🙂

Suggested change
string $copyIdentifier = '',
?string $copyIdentifier = null,

@github-actions
Copy link

Version

Target Version 53.4.1
Last version 53.4.0

There are 0 BREAKING CHANGE, 0 feature, 5 fixes

@hectoras
Copy link
Contributor

hectoras commented Jul 26, 2023

TL;DR: Maybe this can be solved by simply declaring ResourceTransferProxy in the DI contanier as non-shared.

I think this happens because the class copier is instantiated once by a given worker and then it "remembers" the IDs for the classes it copied for prior requests. And I think this happens because the service is instantiated once from the DI container and then reused for each task run by the worker, so wouldn't it be easier to just declare the services form the DI container as non-shared so their state is not remembered across invocations (here)?

Specifically, I think this may be happening because ResourceTransferProxy receives a ClassCopierProxy instance when it is constructed: ClassCopierProxy is non-shared, but ResourceTransferProxy is, so it retains a reference to the previous ClassCopierProxy instance either way.

Note here the problem is caused by having a service that is statefull (ClassCopier having $copiedClasses) and we are trying to solve the issue by adding more state (which "operation" the copy belongs to). There is another problem (that was there before) related to memory usage: A copier instantiated by a worker will be increasing the worker memory footprint each time is invoked (by storing the copied classes and now also an additional operation ID per copy operation).

@hectoras hectoras self-requested a review July 26, 2023 11:49
@tikhanovichA
Copy link
Contributor Author

Will be closed for solving it as @hectoras proposed.

@tikhanovichA tikhanovichA deleted the fix/ADF-1545/copy-of-copied-class branch July 26, 2023 12:07
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.

5 participants