Bug #6541
repostory: unlink objects failure
0%
Description
when unlinking objects in the repository: "Your call to the method escape_column_name in repository\ColumnNameDataManager could not be resolved."
Files
History
Updated by Anonymous over 6 years ago
- Status changed from New to Needs testing
the method escape_column_name is called in many different ways:
\repository\DataManager :: escape_column_name \repository\DataManager :: get_instance() -> escape_column_name $this->escape_column_name
The first one is erroneous, and appears in files
application/elude/php/lib/data_manager/mdb2.class.php application/gutenberg/php/lib/data_manager/mdb2.class.php application/internship_organizer/php/lib/data_manager/database_internship_organizer_data_manager.class.php application/internship_organizer/php/lib/data_manager/mdb2.class.php application/package/php/lib/data_manager/mdb2.class.php application/photo_gallery/php/lib/data_manager/mdb2.class.php application/phrases/php/lib/data_manager/mdb2.class.php application/profiler/php/lib/data_manager/mdb2.class.php application/wiki/php/lib/data_manager/mdb2.class.php
The only places where a method escape_column_name is defined are in the Dcotrine and Mdb2 databse classes. I believe the correct use is therefor
$this->escape_column_name
in the derived classes.
Updated by Anonymous over 6 years ago
I failed to see the method escape_column_name is static. The most correct use is: Mdb2Database :: escape_column_name, or DoctrineDatabase :: escape_column_name.
However, given PHP's non-strict evaluation, calling a static method non-statically is harmless.
Updated by Anonymous over 6 years ago
- Status changed from Needs more info to Bug resolved
fixed
removed all incorrect \repository\DataManager :: escape_column_name calls and replaced with Mdb2Database :: escape_column_name
Updated by Sven Vanpoucke over 6 years ago
Updated by 
- Status changed from Bug resolved to Assigned
- Assignee set to Anonymous
Never replace a call to the general DataManager to one of it's implementations. You should always call DataManager :: get_instance() if you need to call a function of your implementation (depending on the currently selected implementation in the config).
Please revert this last change to \repository\DataManager :: get_instance()->escape_column_name
Updated by Anonymous over 6 years ago
What should I do with calls like $this->escape_column_name()? They are all over the place
Updated by Anonymous over 6 years ago
And why is escape_column_name a static method? One needs to call it through a singleton, while the datamanagers extend directly from the Mdb2 or Doctrine base classes? This seems like a useless detour, adding complexity but no functionality.
The codebase contains a lot of calls to DoctrineDatabase :: escape_column_name, which I took as a reference for this bugfix. Should I replace all of them too?
Updated by Sven Vanpoucke over 6 years ago
There is a difference from what source you are working:
If you are working from an MDB2 or Doctrine implementation you should call parent :: escape_column_name since it's an extension
If you are working from the general DataManager you should call $this->get-instance()->escape_column_name
If you are working from an external source like a component you should call DataManager :: get_instance()->escape_column_name
But never reference to the MDB2Database or DoctrineDatabase directly.
Updated by Anonymous over 6 years ago
it remains inconsistent: why is is called statically in the first case and non-statically in the other cases?
Updated by Sven Vanpoucke over 6 years ago
Because the DataManager class is basically a delegation class with some additional functionality (like caching), which redirects most of it's logic to the selected implementation through the Singleton pattern. This is a static class and therefor the delegation function for the escape_column_name is also static. It is there so you can call this function without the need to execute the get_instance(). Basically it's just a facade for DataManager :: get_instance()->escape_column_name
Updated by Anonymous over 6 years ago
I guess that classes like common/libraries/php/shared/storage/implementation/doctrine/condition/equality_condition.class.php can still use DoctrineDatabase :: escape_column_name, as they are by definition bound to Doctrine? (analogous for Mdb2).
I am currently fixing 1166 instances of the escape_column_name problem. It seems unreasonable that all of them are wrong, however they do not comply to the rules you explained above.
Updated by Sven Vanpoucke over 6 years ago
They are core classes which are already inside the doctrine implementation so they obviously belong to the doctrine implementation and therefore these classes are the only ones that do not need to comply to the rules above. But since this does not belong to the broken functionality, you should not bother looking into these classes.
Updated by Anonymous over 6 years ago
I am still not satisfied: the only reason it works with this scheme is because PHP allows to call static functions in a non-static way. I fail to see the point of mixing static and non-static. Anyway, I am not in a position to question the design, I'm just pointing out the inconsistency.