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(modulebuilder): validate after creation #33105

Merged
merged 6 commits into from
Feb 18, 2025
Merged

Conversation

BenjaminFlr
Copy link
Contributor

CommonObject::createCommon() returns the id of the newly created object, and we store it in $resultcreate.
MyObject::validate() returns an integer between -1 and 1.

If we overwrite the value of $resultcreate with -1, 0 or 1 instead of the object's id, the return of MyObject::create() is necessarily false. That's why I changed the name of the variable for the result of $this->validate().

@hregis
Copy link
Contributor

hregis commented Feb 17, 2025

@BenjaminFlr and what is the impact of renaming a variable "$resultcreate" into "$resultvalidate" in a comment that is not returned in the end?

return $resultcreate;

@eldy
Copy link
Member

eldy commented Feb 17, 2025

I suggest a more complete fix that handle both create and validate:
2c7b2a1

Is it ok for you @BenjaminFlr

@hregis
Copy link
Contributor

hregis commented Feb 17, 2025

That's where my limits are!
Where I only bring reproaches, Laurent brings a solution!
That's why he's a project manager! 😄
I need to call my psychiatrist !

@BenjaminFlr
Copy link
Contributor Author

BenjaminFlr commented Feb 17, 2025

To answer to both of you @eldy and @hregis
If I don't want to rewrite a huge part of the code, we can't return the result of $this->validate() because the returned value is the one used in substitution __ID__ for $backtopage in myobject_class.php :
https://github.com/Dolibarr/dolibarr/blob/28cacb8d1ab612696bd092840c8a4f99c20527f0/htdocs/modulebuilder/template/myobject_card.php#L198C1-L207C1

Also, if you return the result of $this->validate() and everything goes fine, you'll always go to the card with ?id=1.

That's why I proposed to just change the name of the variable and, then, everybody can make his own tests depending on his needs. Because in my case, if the validation fails, I don't want to delete the object. But another developer could.

@hregis
Copy link
Contributor

hregis commented Feb 17, 2025

@BenjaminFlr excuse moi, quand je ne comprend rien je préfère le faire en français... je n'ai rien compris à ce que tu as dis ! :-)
je ne remet pas en doute ta capacité à parler la langue de Shakespeare, c'est juste que je n'ai pas d'abonnement à ChatGPT et Google Translator c'est de la merde ! ;-)

@BenjaminFlr
Copy link
Contributor Author

@hregis en français :

Si je ne veux pas faire une grosse réécriture de code, on ne peut pas retourner le résultat de $this->validate() parce que la valeur retournée dans la fonction myObject::create() est celle qui est utilisée pour la chaîne de substitution __ID__ de la variable $backtopage dans myobject_class.php :
https://github.com/Dolibarr/dolibarr/blob/28cacb8d1ab612696bd092840c8a4f99c20527f0/htdocs/modulebuilder/template/myobject_card.php#L198C1-L207C1

Donc si tu retournes toujours le résultat de $this->validate() et que tout se passe bien, tu auras toujours 1 dans la chaîne de substitution, et donc tu iras toujours dans la card avec ?id=1 au lieu de celle que tu viens de créer

C'est pour ça que j'ai proposé de ne changer que le nom de la variable pour le $this->validate(). Ensuite, chacun fait ce qu'il veut de cette valeur en fonction de son besoin. Dans mon cas, si la validation échoue (et retourne 0 ou -1), je m'en fous, je veux pas supprimer l'objet créé. Mais une autre personne aura peut-être un autre traitement à prévoir si le validate échoue.

@hregis
Copy link
Contributor

hregis commented Feb 17, 2025

@BenjaminFlr et ce qu'a envoyé Laurent comme correction ce n'est pas ok ?

@BenjaminFlr
Copy link
Contributor Author

@hregis si je suis pas passé à côté de quelque chose, non :

	public function create(User $user, $notrigger = 0)
	{
		$result = $this->createCommon($user, $notrigger);

		// uncomment lines below if you want to validate object after creation
		// if ($result > 0) {
		// $this->fetch($this->id); // needed to retrieve some fields (ie date_creation for masked ref)
		// $result= $this->validate($user, $notrigger);
		// }

		return $result;
	}

Si on fait ça, ça ne change rien, on remplace juste $resultcreate par $result

@hregis
Copy link
Contributor

hregis commented Feb 18, 2025

@BenjaminFlr peut-être ajouter un test sur le "$resultvalidate" et si ok on renvoi le "$resultcreate" avec l'id de l'object ?
mais bon c'est du code exemple.
au final tu as raison, il faut différencier les deux "$result" pour ne pas écraser l'id du "create" (tu l'as tout de même dans le "$this->id" normalement !)

@eldy eldy merged commit 0d10942 into Dolibarr:develop Feb 18, 2025
3 of 4 checks passed
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.

3 participants