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

#29026 [Form] fix: selectForForms issue for manage case where objectdesc don't need to reset #30715

Open
wants to merge 1 commit into
base: 19.0
Choose a base branch
from

Conversation

nicolas-eoxia
Copy link
Contributor

@nicolas-eoxia nicolas-eoxia commented Aug 21, 2024

Field from class we want use selectForForms functions
image

Error occured because type integer not integer:Product......
image

Fix with check and reset value
image

Add preg_match for check before preg_replace
Add reset objectdesc to default value

… objectdesc don't need to reset

Add preg_match for check before preg_replace
Add reset objectdesc to default value
@nicolas-eoxia nicolas-eoxia self-assigned this Aug 21, 2024
@nicolas-eoxia nicolas-eoxia added the Bug This is a bug (something does not work as expected) label Aug 21, 2024
@eldy
Copy link
Member

eldy commented Aug 22, 2024

Can you paste the code of ->fields inside the object "session formation" ?

@eldy eldy added the Bug or PR need more information This bug or PR needs more information (answer to a question or more accurate description) label Aug 22, 2024
@nicolas-eoxia
Copy link
Contributor Author

nicolas-eoxia commented Aug 22, 2024

All fields from Session class
image

Indeed fk_element type is integer but in create or edit action i want upate fk_element type by this on capture
image

The reel problem here is on selectForFroms functions L8050

$objectforfieldstmp = fetchObjectByElement(0, strtolower($tmparray[0]));

the object is retrieved without taking into account modifications to the current object and with id = 0
As a result, after this line we have a fresh object

@nicolas-eoxia nicolas-eoxia requested review from eldy and removed request for eldy August 28, 2024 09:52
@eldy
Copy link
Member

eldy commented Sep 5, 2024

OK. My understanding is that in your object SessionFormation, you set dynamically the array fields to have the correct definition of the link "Service". Not common, and not designed for initially but why not. However, if we are able to have code working for the link when definition is static, we should be able to have same result (code working) when code is dynamically set, with no need to change code. This is why i think fix is suspicious.

fetchObjectByElement(0, strtolower($tmparray[0])); is done with id = 0 because all what we want is an instance of object of the good type (so for you you should get "Product") to be able to reuse later the ->$fields;

What do you have for value in
$objectdesc
$objectfield
$tmparray[0]
$tmparray[1]

@nicolas-eoxia
Copy link
Contributor Author

nicolas-eoxia commented Sep 12, 2024

Here are the values of the variables requested

I agree with the fact that defining a fields link statically or dynamically should not lead to modification, but the problem comes from the fresh object, which doesn't use dynamic modification.

The fix I'm proposing makes it possible to check that there is a link before making a replacement

Otherwise you reset to the initial value

$objectdesc => Product L8044
$objectfield => trainingsession@dolimeet:fk_element L8045
$tmparray[0] => trainingsession@dolimeet L8050
$tmparray[1] => fk_element L8050

$objectforfieldstmp =>

Trainingsession Object ->
[fk_element] => Array
(
[type] => integer
[label] => FkElement
[enabled] => 1
[position] => 92
[notnull] => 0
[visible] => 1
[index] => 1
[css] => maxwidth500 widthcentpercentminusxx
[csslist] => left
)

@nicolas-eoxia nicolas-eoxia changed the base branch from 19.0 to develop September 13, 2024 09:04
@FHenry
Copy link
Member

FHenry commented Sep 13, 2024

@eldy We've change base branch.
What do you think ?

EDIT: finaly we reset to 19.0 because we prefer wait your input

@nicolas-eoxia nicolas-eoxia changed the base branch from develop to 19.0 September 13, 2024 09:31
@eldy
Copy link
Member

eldy commented Sep 13, 2024

OK.
So the entry parameter are correct.
$objectdesc => Product L8044
$objectfield => trainingsession@dolimeet:fk_element L8045

The goal of the selectForForms is to use the field key "fk_element" extracted from $objectfield to retreive into trainginsession@dolimeet->fields["fk_element"] what is the object we want for the combo list (the parent object of fk_element. Here, we hope to get Product).

Here it fails because selectForForms is calling fetchObjectByElement('trainingsession@dolimeet') to get the parent object from the defintion inside TrainingSession, but fetchObjectbyelement is not working on the dynamic value you modified before, it makes a new $... so the object is a new one and the ->fields inside is also a new one, not modified dynamically, so not in phase.

So, for me, the trouble should not be fixed on line 8070, but at the nearest line (the first line) where the deviance appears, so into fetchObjectByElement(), because fetchObjectByElement() return a data with no difference when you made dynamic modifications or not, you are later in the sh.... So later, you introduce some tests to retreive the correct information differently, but with no specific parameter, this let me think this decision will also occurs when it should not.

So i suggest to try this:

First, I suggest into your trainginsession class file, into ->fields['fk_element'] instead of having
... 'type' => 'integer', ...
I suggest to have
... 'type' => 'integer:NotPredeclaredClass:notpredeclaredpath'

This syntax is the one expected by dolibarr now, and the one when generating an object with module builder (except that instead of the name of class you put the hardcoded string "NotPredeclaredClass".
Like this, things are clear we are in a situation we want to use a parent object that is dynamically defined. And we will be able to adapt the framework to be able to manage this case with no side effect for common cases.

Then into the selectForForms, you can change the code

if (empty($objectdesc)) {
					$objectdesc = $objectdescorig;
}

into

if (strpos("NotPredeclaredClass", $objectdesc) === 0) {
					$objectdesc = $objectdescorig;
}

This way, it is clear that the test to fallback is only for special cases you are in.

Can you try this way ?
I did not test, it is just an idea to validate (we may need to add the test if NotPredeclaredClass at other places in code but at least we will know every places where the management of dynamic fields need special adaptation)...

Note: As this is a developer feature not used in a conventionnel way (it was initially not thinking for dynamic use), we must introduce a solution, but this can only be in develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug or PR need more information This bug or PR needs more information (answer to a question or more accurate description) Bug This is a bug (something does not work as expected)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants