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

Clone assets #6684

Merged
merged 3 commits into from Feb 27, 2020
Merged

Clone assets #6684

merged 3 commits into from Feb 27, 2020

Conversation

flonou
Copy link
Contributor

@flonou flonou commented Dec 6, 2019

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #6682

@flonou flonou changed the base branch from 9.4/bugfixes to 9.5/bugfixes December 6, 2019 18:24
@trasher trasher added this to the Candidate for next major version milestone Dec 10, 2019
@trasher
Copy link
Member

trasher commented Dec 10, 2019

Some simple unit tests must be added within this feature (should test cloning of several objects types). Tell us if you need help on that.

@flonou
Copy link
Contributor Author

flonou commented Dec 11, 2019

Working on tests now

@flonou
Copy link
Contributor Author

flonou commented Dec 18, 2019

@cedric-anne I would like to have more information about the Toolbox::addslashes_deep call. It messes with some fields (for exemple the helpdesk_item_type of profiles is badly cloned). Is this call necessary ?

@trasher
Copy link
Member

trasher commented Dec 19, 2019

addslashes_deep must be used for all data you want to store in the db that are not coming from HTTP requests to prevent quoting issues and/or sql injection issues.

All HTTP request contents ($_POST, $_GET, ...) are already escaped; so for those data you should not call the addslashes method.
This is because GLPI historically sanitize all data it receive (we'd love to change that, but we can't without breaking many things).

If you mix POST'ed data with some values from the database in a same array you want to store, you have to ensure you will not escape twice.

@flonou
Copy link
Contributor Author

flonou commented Dec 19, 2019

Hmm ok. Seems like adding the addSlashes_deep call is messing with some fields on my test instance so I removed it but the test is still failing

@trasher
Copy link
Member

trasher commented Dec 19, 2019

I will not have time to dig into this; but as far as I see, your issue comes from a "dbarray" that is handled in a specific manner in glpi (see importArrayFromDB and maybe Toolbox:decodeArrayFromInput).

@flonou
Copy link
Contributor Author

flonou commented Dec 19, 2019

Ok I removed the clone ability on profiles because the add method resets the rights, I guessed for security reasons. I need to add tests on non assets types to ensure they all work correctly

@flonou
Copy link
Contributor Author

flonou commented Dec 19, 2019

Note : some items (specifically CommonDBChild) have a cloneItem method which is used by their parent containers to clone them when using templates. However, this method is not the one called when cloning the child inside the parent by my method (ex: cloning a network port). Should I try to merge both behaviors ?

Copy link
Member

@trasher trasher left a comment

Choose a reason for hiding this comment

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

Just made a technical review of the code, this seems correct; good job 👍
I'll do a functional review asap.

I did not understand your last comment, I'm not sure this is still relevant.

@flonou
Copy link
Contributor Author

flonou commented Jan 8, 2020

@trasher Some classes already have a clone method named cloneItem. Specifically for templates I guess (when you instantiate a template, the template is cloned). Seems like many commondbchild subclasses need this to clone their subelements.
One example is the networkport class that needs to clone the instantiation it's linked to. This clone method is called in the post_additem of computer.class for example.

My instantiation also need this cloning of the subitems (done in the post_clone method in my case)

I was wondering if I should leave the cloning methods used for templates or try to homogenize it with my contribution

@trasher
Copy link
Member

trasher commented Jan 8, 2020

I was wondering if I should leave the cloning methods used for templates or try to homogenize it with my contribution

Oh OK; thanks for the explanation :)
It sounds indeed a good idea to use existing methods, and this should not require too much work on your branch as far as I see.

@vollkommenIrrelevant
Copy link

vollkommenIrrelevant commented Jan 23, 2020

@flonou
very cool! Thanks a lot for your work!

@trasher
maybe someone can Review this in time - I have to clone a lot ;-)

@trasher
Copy link
Member

trasher commented Jan 23, 2020

I'm waiting for @flonou to homogenize th code as it was proposed

@vollkommenIrrelevant
Copy link

@flonou
come on ;-)

@flonou
Copy link
Contributor Author

flonou commented Jan 28, 2020

H!
I'm quite busy at work these days so this is going slowly.
For now I'm trying to find a way to tell a CommonDBChild or CommonDBRelation to change its item_id (the item it is a device of).

@flonou
Copy link
Contributor Author

flonou commented Jan 29, 2020

@trasher Hi, I'm trying to get rid of the cloneItem methods and replace them with my clone method but I'm having trouble finding out how to update CommonDBRelation items.
If for example I clone a computer, I need to clone the item_operatingsystem and item_devices it is linked to.
I planned to do that using the affectRelation, but this method requires me to tell which peer I want to update (0 or 1). However, it seems that for some of these classes, the peer I need to update is 0 (for devices) or 1 (for operatingsystem).

An alternative way (which would require less modifications on my part) of doing the same thing (meaning cloning sub-elements of a computer for example) would be for me to call the cloneItem methods when trying to clone a CommonDBRelation or CommonDBChild but I'm not quite convinced that these methods a nicely implemented in the sense that they are not part of the interface of CommonDBRelation or CommonDBChild. They are just declared in the sub-classes which seems prone to inconsistency if new sub-classes need to be added. Also I don't know if plugins rely on these methods or not.

EDIT : A third alternative I'm thinking about would be to have an optional parameter in my clone method where we could override parts of the input parameters copied from the object to be cloned (in my case, I would override 'items_id' and 'itemtype')

@flonou
Copy link
Contributor Author

flonou commented Feb 3, 2020

I went with the third option in the end, because otherwise, cloning would be blocked due to unicity of some parameters list in the database

@trasher
Copy link
Member

trasher commented Feb 4, 2020

Hi @flonou;

Tests are currently failing because you deprecate the cloneItem method, but there are still usages of it in the code. You have to replace all cloneItem calls ;)

@trasher
Copy link
Member

trasher commented Feb 4, 2020

Also, you have a lot of CS issues than can be fixed with phpcbf:

./vendor/bin/phpcbf -d memory_limit=1G -p --standard=vendor/glpi-project/coding-standard/GlpiStandard/ inc/

@flonou
Copy link
Contributor Author

flonou commented Feb 4, 2020

Yes, the cloneItem method is used directly in the tests. I also need to merge the targeted branch in the process because it has changed. Should be done this week

@flonou
Copy link
Contributor Author

flonou commented Feb 5, 2020

So I have two tests failing and I'm not sure if I should change them or not.

  • The first on is in itil_project.php, It's adding an object with an _oldID parameter and expects to have a clone of the object. Should I support this way of cloning ?

  • The second one is in Software.php. It's adding an object with an id parameter and expects to have a clone of the template with this id. As above, should I support this way of instancing a template ?

@trasher
Copy link
Member

trasher commented Feb 6, 2020

So I have two tests failing and I'm not sure if I should change them or not.

* The first on is in itil_project.php, It's adding an object with an _oldID parameter and expects to have a clone of the object. Should I support this way of cloning ?

As far as I know, this is not the only place where you'll find an _oldID that would result in a clone. So, yes, I think it should be supported.

* The second one is in Software.php. It's adding an object with an id parameter and expects to have a clone of the template with this id. As above, should I support this way of instancing a template ?

This seems to be a "non standard" use case; it should work, but only for Software I guess. I do not have in mind any other cases like this one; maybe @cedric-anne or @orthagh would have a point of view on this one.

inc/config.php Outdated Show resolved Hide resolved
@trasher
Copy link
Member

trasher commented Feb 10, 2020

You should take care your database is up to date, this may cause difficult to find issues.
Indeed, update of dev branch is not supported, but from the UI only, but you can use cli commands, see https://glpi-install.readthedocs.io/en/develop/command-line.html#update

@flonou
Copy link
Contributor Author

flonou commented Feb 13, 2020

Yeah I tried to use the cli commands but after using the ui so it was somehow blocked. I must have messed up somewhere

Copy link
Member

@trasher trasher left a comment

Choose a reason for hiding this comment

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

Hi,

This sounds correct, thank you :)

Just a few points left:

  • you should add the @deprecated 9.5 on phpdoc for methods that are deprecated,
  • you should list deprecated methods in changelog file.

@trasher trasher requested a review from orthagh February 26, 2020 08:40
@trasher
Copy link
Member

trasher commented Feb 26, 2020

@vollkommenIrrelevant this PR is almost finished now I guess, maybe could you test the feature and give us feedbacks. Thank you.

@vollkommenIrrelevant
Copy link

@trasher
unfortenately I have no testing environment. Is there a release which is sufficent stabel for production usage?

@trasher
Copy link
Member

trasher commented Feb 26, 2020

This is a work in progress; nothing is stable as it need to be tested.

@vollkommenIrrelevant
Copy link

ok, ich will check for a solution.
how can I download the archive?

@trasher
Copy link
Member

trasher commented Feb 26, 2020

ok, ich will check for a solution.
how can I download the archive?

You can download sources archive from @flonou branch (clone or dowload => dowload zip):
https://github.com/flonou/glpi/tree/clone_assets

As said, it's source; not release. Main difference is third party libs (php, js, etc) are not provided, you have to install them, see https://github.com/glpi-project/glpi/blob/9.5/bugfixes/INSTALL.md

Do not use this branch on production!

inc/item_devices.class.php Outdated Show resolved Hide resolved
@trasher
Copy link
Member

trasher commented Feb 26, 2020

Tests are now failing because of the merge I've done to fix the conflict on changelog file. Dashboards feature has been recently merge in the 9.5 branch, and I had not in mind it would break here.
Anyways, there are some fix to do; can you work on it @flonou? I probably won't have time until next week.

@flonou
Copy link
Contributor Author

flonou commented Feb 26, 2020

I guess it would be better to not redefine the clone method but instead prepareInputForClone() ?
Or maybe rename the method? (clone is supposed to return an ID to the cloned object, not an array)

@orthagh
Copy link
Contributor

orthagh commented Feb 26, 2020

ok, i'll redo this, a new method is preferable i think

@flonou
Copy link
Contributor Author

flonou commented Feb 26, 2020

Yes, I've had a look, it seems like the dashboard class is not quite behaving as a standard CommonDBTM (no use of add for example) and clone relies on this (although you can completely redefine the behaviour of clone if needed)

Copy link
Member

@trasher trasher left a comment

Choose a reason for hiding this comment

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

Thank you! :)

First implementation of the clone functionality
Updating tests to take into account the new action
removing missed debug log
Fixing code quality issues
Fixing test case, we now have one more action possible
Simplifying the clone method
Adding test for the clone method on various types of items
error in test code
same ; missing on another line
fixing code   issues
Adding a prepareInputForClone method that's useful for some types (user for example)
Fixing the tests
typo
fixing some code quality issues and tests
date to string comparison is not working
getting a profile item was not correctly done (was trying to get a user instead)
moving prepareInputForClone before the addslashes_deep method
Trying to get the date fields comparisons right
Still working on date comparison
maybe dates can't be compared ?
turns out date is not outputing a date but a string !
Fixing the cloning issue
Removing clone capability on profiles because rights are automatically unset after add and that might be a security issue
updating test case
adding more test cases and cloning capabilities
working on tests
trying to get more information on why a test fails
updating cloning
Fixing code identation
empty line fixing
Homogenization of cloning with the new method (removing calls to cloneItem methods for templates instanciation)
Handling legacy cloning requests (calling add with id or _oldID set)
Fixing code quality
Adding @SInCE and @deprecated markers in the phpdoc
Updating the CHANGELOG.md file
Fix cs
fix defition on Dashboard::clone
rename method
Fixing dashboard test
@trasher
Copy link
Member

trasher commented Feb 27, 2020

i've rebased+squashed the PR, I was not able to merge it from GH inteface

@trasher trasher merged commit 1de713e into glpi-project:9.5/bugfixes Feb 27, 2020
@vollkommenIrrelevant
Copy link

due to lack of infrastructure no testing possible, sorry.

@trasher trasher removed this from the Candidate for next major version milestone Jun 23, 2020
@Mirkk Mirkk mentioned this pull request Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants