User tests: Successful: Unsuccessful:
Based on the problem of saving data with different types.
[4.0] Error when saving an object in JTable->save()
I've done a great job of implementing a typizations sub-framework for a Table
class.
The problem is that before you could not create properties of the Table
class with different types, you were forced to create properties without types. You had to use the default types.
But now in the modern world, you can use the Table
class with fully typed properties. You don't need to worry that a DateTime property will cause an error. The DateTime property and other properties will be automatically converted to the database type before being saved. When the Table
is loaded, all properties will be automatically converted to objects, such as a DateTime object.
Now, when programming, you will focus only on business logic.
.
At the same time, the updated Table
class supports the previous usage scenarios. You don't need to redo the programs you've already created.
.
Bonus: The update made it possible to stop using the methods of the deprecated CMSObject class. The new Table
class still inherits the CMSObject class, but now no method is used from this parent class.
.
The new Table
class will allow you to discard the parent CMSObject class without loss in the future.
I checked this class, it does not violate the CMS in any way. But I still think that the code requires minor optimization.
.
I ask you not to ban my work, but to help solve the problem of complete typizations of classes, since technologies do not stand still.
In the modern world, typizations is a basic feature for frameworks.
The new typed Table class requires a minimum PHP 7.4
To use this class, you need to raise the minimum version of PHP in Joomla 4.1
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
Labels |
Added:
?
|
Can we create another new Table class and place it next to the previous one? We will call it by another name, for example: TableTypes
.
This way we will not violate minor version compatibility. We will be able to test this feature and find out the pitfalls when converting types.
The previous correspondence is located in #34589
It would be convenient to create an additional Table Types class, but inherited from the Table class.
So that there are no errors when checking the parent.
Let the community mentors tell me:
this seems to have broken some unit tests - please see https://ci.joomla.org/joomla/joomla-cms/46866
No the tests are working perfectly. This code will not work on php 7.2 or 7.3 so the test results are correct
Well as the minimum PHP version for Joomla 4 is set at PHP 7.2.5 - this PR cannot be accepted.
https://downloads.joomla.org/technical-requirements
The next raise of PHP Minimum version would be in Joomla 5.0.0 - not Joomla 4.1.0.
actually the production department changed the rules on that with #PROD2020/021
Joomla's current policy is that we only accept changes or features that work for everything that meets our minimum requirement (e.g. PHP versions, browsers, database versions etc..)
The motion goes beyond this by allowing progressive features. This allows us to take advantage of new technology for those that use it as long as there is no adverse effect to those that don't (this should also take into account the support/documentation affects on such conditional features). As an example we could look at
Not enough text in that to actually understand what was proposed, voted on and agreed.
"This allows us to take advantage of new technology for those that use it as long as there is no adverse effect to those that don't "
This means that it needs to work on PHP 7.2, but could work differently for those on PHP 8. But those on PHP 7.2 should still be able to have "no adverse effect" - I.e the tests still need to work on PHP 7.2 and a Joomla 4.1.0 site on PHP 7.2 should not fail due to this PR.
The fact is that if this were to be merged in Joomla 4.1.0 then that would be breaking the commitment Joomla has already made, and the documented technical requirements of the whole "Joomla 4.x" series.
we can add the TableTypes class marked depricated. or use another label to show the possibility of using it in J5. In Joomla, there are always deprecated methods and classes that are not recommended for use. We can do the same.
In Joomla, there are always deprecated methods and classes that are not recommended for use. We can do the same.
Yes but you cannot run introduced in PHP 7.3, in Joomla 4, unless that code is optionally called and by not calling it has no adverse effect to those that
run PHP 7.2.5
Joomla 4.x has to remain compatible at all times with the published requirement of PHP 7.2.5
so let's call the code additionally. we will make the proposed changes to another class Tabletypes
, which will be called additionally.
.
@PhilETaylor @brianteeman
colleagues. I don't solve my problem with tables. I have already solved my problem. I want to contribute to the community. I understand the backward compatibility policy. I understand the subtleties of politics less than you do. I ask you to write here on the thoughts, "not why it can't be done". And concentrate on the thought " Why it can be done. How to make it possible to introduce the possibility of new typing, and at the same time comply with the policy without violating the principles."
Quite simply, you cannot introduce PHP 7.4 features to Joomla 4.x unless those features are only optionally run on PHP 7.4, but still allow Joomla to run on PHP 7.2.5 without any adverse effect
to those that choose to run that end of life PHP version.
At all times the code must be compatible and run on PHP 7.2.5.
Unless the maintainers of Joomla change their policy.
At all times the code must be compatible and run on PHP 7.2.5.
Tell me please.
Can the code be modified so that there can be a condition in the code? The condition will check the PHP version, if the PHP version is 7.4, then type checking will be performed, if the version is lower than 7.4, then there will be no type checking.
But if I modify the code in this way, the code will be compatible with php 7.2.5. But will it be able to pass the tests?
Due to the condition of checking the PHP version, and executing different statements, the code will meet all the requirements of the backward compatibility policy.
if its written correctly then the tests should pass - yes.
Here is an good example of by allowing progressive features. This allows us to take advantage of new technology for those that use it as long as there is no adverse effect to those that don't
where the ReflectionUnionType
type is actually only available in PHP 8 and the foreach
will only run in PHP 8+, but this code is perfectly compatible with PHP 7.2.5 because it will never ever run in PHP 7.2.5 as ReflectionUnionType
will never exist. There is no adverse effect
on those running PHP 7.2.5, they just dont get the additional checks run.
joomla-cms/libraries/src/Plugin/CMSPlugin.php
Line 346 in 55b02c9
which is
// Handle PHP 8 union types.
if ($reflectionType instanceof \ReflectionUnionType)
{
foreach ($reflectionType->getTypes() as $type)
{
if (!\is_a($type->getName(), EventInterface::class, true))
{
return false;
}
}
return true;
}
Thank you for this PR! I'm expecting the PHP 7.2.5 compatibility won't let this PR through right now but I'm looking forward to when we can allow these changes with a bump in the PHP requirements.
Labels |
Added:
?
Removed: ? |
Labels |
Added:
?
Removed: ? |
@richard67
Tell me please.
I got confused in the error log.
This is an error due to my fault, and I also need to fix the code. Or the test drone made a mistake in conducting the test.?
I don't know. I've restarted it to see if it works this time.
I found an error. I'm correcting it.
@ditsuke @PhilETaylor
Colleagues, please help!
I have done a lot of tests, on different versions of PHP 7.2-8.
Everything works fine.
Help to understand the error that the drone finds.
In the description of the error, it says that you can see the screenshot, I do not know where to watch it.
And it is not clear with the use of which code the error is caused.
. On the other hand, the new table class may contain an error.
But that's why I'm asking you to test this class.
I ask you to help me find an error in the text of the drone.
https://ci.joomla.org/artifacts/joomla/joomla-cms/4.1-dev/35247/system-tests/47299
Could not refresh manifest cache for extension: com_wrapper
@korenevskiy You have to scroll down the list on the left side of the drone results until you see a step called "artifacts-system-tests". Select this and then scroll up the text area at the right hand side. You should see this:
export PLUGIN_DEST_DIR=$PLUGIN_DEST_DIR/$DRONE_REPO/$DRONE_BRANCH/$DRONE_PULL_REQUEST/system-tests/$DRONE_BUILD_NUMBER
echo https://ci.joomla.org$PLUGIN_DEST_DIR
https://ci.joomla.org/artifacts/joomla/joomla-cms/4.1-dev/35247/system-tests/47299
...
The link leads to a page where you can see a link to the screenshot (png) and the html page:
The PNG then shows:
@ricardo1709 @PhilETaylor
Welcome. Please tell me how to enable debugging mode in INSTALLATION mode when installing Joomla?
So that the debug panel with information about SQL queries at the time of installation is displayed at the bottom.
I've been sitting for many days and trying to figure out at what point NULL turns into an empty string.
In the new table class, I see that there are no transformations. But at the output, NULL turns into an empty string.
I need the debug panel.
joomla-cms/installation/localise.xml
Line 4 in c70520d
joomla-cms/installation/localise.xml
Line 4 in c70520d
This does not add a panel with SQL. It just allows you to debug localization files.
@richard67
Help.
I looked at the link artifacts-system-tests
in the debug.
But the link does not open.
Can you tell me where to get the error data?
@richard67
Help.
I looked at the linkartifacts-system-tests
in the debug.
But the link does not open.
Can you tell me where to get the error data?
The log file is a text file, and text files don't include hyperlinks. So you have to copy the URL from the text file and paste it into the URL field of your browser.
@richard67
Help.
I looked at the linkartifacts-system-tests
in the debug.
But the link does not open.
Can you tell me where to get the error data?The log file is a text file, and text files don't include hyperlinks. So you have to copy the URL from the text file and paste it into the URL field of your browser.
I do this of course. but the link opens an empty page.
@korenevskiy I see ... the reason is that the tests fail at the integration tests, which run before the system tests, so we have o results from the system tests. The integration tests log you can see e.g. here: https://ci.joomla.org/joomla/joomla-cms/47482/1/12 . It shows that the tests fail because they get a result '0' where it should be null. So either your code changes something which is not b/c, or the tests need to be changed, too. Maybe that log tests you where the problem is. I'm at paid work now and so don't have time to dig deeper. Good luck.
@richard67
Hello.
Please give me some advice.
I will not ask you to find the cause of the error in tests or code.
I have NO complaints about the tests.
But please advise how to find the property that changes from 0 to NULL.
Is it possible to determine this somehow without delving into the test code?
And the following was written in the error:
Joomla\Tests\Integration\Libraries\Cms\Table\TableTest::testBindIgnoresFields
Failed asserting that '0' matches expected null.
/********/src/tests/Integration/Libraries/Cms/Table/TableTest.php:171
I found a repository and a file in it:
https://github.com/joomla/test-integration/blob/master/suites/libraries/joomla/table/JTableTest.php
But I'm in the file JTableTest.php he did not see anything about the error in line 171.
Because as 171 lines indicates the end of the function.
Please tell me, I found the file correctly and it is in this file that the error lies in line 171, or do I need to look in some other file in the other repository?
@korenevskiy You have to find the test which includes the failed "testBindIgnoresFields". After 1 minute search I found this: https://github.com/joomla/joomla-cms/blob/4.1-dev/tests/Integration/Libraries/Cms/Table/TableTest.php#L154 . In that function you find the data and the asserts which are checked. As you can see, the test deals with the ignore fields of the "bind" method. Ignored fields should be null when having been removed. I hope that helps.
@korenevskiy Other question: Have you ever tested what your code does with a PostgreSQL database?
@korenevskiy I think the issue is in what you are trying to do with default values. When a column is present in database but not in the object to be written to database, you want so use the default value, as far as I understand what you are trying to do. That is not right because a bind can be used for different statement, e.g. insert or update. When inserting new data, default values will be used when set for a column and the column is not in the statement. But when updating data, default values have no purpose and shall not be used. If a column is not in the statement, its value shall not change in database.
When columns are removed from a bind because they are in the ignore fields, they should correctly have a value of null in PHP (or be removed from the object, too, in advance), but it seems your code sets them to zero when they are integer. This causes the test to fail.
So I think your PR here is doing wrong stuff. Maybe even the basic conception is wrong.
My other question about PostgreSQL was because you are using "datetime" as SQL type name, but in PostgreSQL there is no such type, there is timestamp without time zone.
@korenevskiy Other question: Have you ever tested what your code does with a PostgreSQL database?
not tested
@richard67
Thank you very much for your help.
You've been very helpful.
The test is passed.
Now there is a test API error.
Thanks to you, I understood how Unit tests work.
Now I will study the Joomla API to initiate an external user API request and get a similar error.
@brianteeman
The last test of this class.
Test API error.
I was looking on the Internet how the Joomla 4 WebService works.
I found this short instruction.
Joomla 4 tutorial: Using the Web Services API 10min
Super video.
I did everything as in the video.
but the API returns the response {"errors":[{"title":"Resource not found","code":404}]}
Have you had any experience working with Joomla WebService?
1.I downloaded the PostMan program.
2. Downloaded profiles for PostMan from https://github.com/alexandreelise/j4x-api-collection
3. I registered the variable {{base_path}} to the local address http://gesor
4. I registered the J4 user token as Authorization-Bearer Token.
5. I'm sending requests {{base_path}}/api/index. php/v1/content/article/10
But every time an error is returned.
@richard67
When the idea to create this class came up, I thought about what kind of implementation method can be used for the class.
It seemed to me that it was possible to go different ways. Since I had a general idea about the work of the class in my head.
But there were various subtleties in the programming process. Considering which it became clear that the development of a class with type support would be only such, and no other.
In this case, the options are possible in detail,
At the very beginning, it seemed to me that there are various options for creating this class, but if we take into account all the subtleties, I come to the conclusion that there are no other options.
@korenevskiy there nothing to do with node, one of your changes crashes test for ContentCest.php:testCrudOnArticle.
But do not lose your time.
Take my advice and close this PR, I doubt it will be accepted, because it not a right way to do what you doing here.
If you want to know how "real typization" work, look for example how it done in DBAL
@korenevskiy there nothing to do with node, one of your changes crashes test for ContentCest.php:testCrudOnArticle.
But do not lose your time. Take my advice and close this PR, I doubt it will be accepted, because it not a right way to do what you doing here. If you want to know how "real typization" work, look for example how it done in DBAL
I wanted that when creating a property with the JDate type, the table could convert this type to the database type itself, and vice versa, convert the date type in the database to the JDate property type.
This is what this table class does. He doesn't do anything special else.
Agree that if a programmer uses a property with the JDate type, he necessarily needs a handler to convert this type into a database type. And this class will do it automatically.
This class removes unnecessary work from the developer. The community created the FrameWork for this very purpose, in order to do some of the work for the programmer automatically.
Thanks for the link.
Thanks to the PHP and database type mapping table, I can complement the class.
I knew that this class does not convert simple types correctly.
Clarifying the type mapping, I was going to do at the next stage of class preparation. The class has a protected static $_typesvaluesproperty,
now I can supplement it with values from the types by your reference DBAL.
Could you tell me where I can see the reason for the test crash?
the table could convert this type to the database type itself
It not a job for Table. It job for DB driver, that is the reason I gave you that link.
But it not something that can be implemented easily, without b.c. break.
Could you tell me where I can see the reason for the test crash?
This seams like a lot of work for Joomla to avoid using Doctrine. Can we please just use Doctrine ORM and DBAL. This solves these problems.
This seams like a lot of work for Joomla to avoid using Doctrine. Can we please just use Doctrine ORM and DBAL. This solves these problems.
@krileon
I was striving for this. But I didn't do full-fledged tests. But going a little deeper into the types. I found out that in PostgreSQL there is a type of integer with a very long length. PHP does not support this length. So there is a long integer problem that cannot be solved by PHP, it has to be solved by the person who designs the database. Such a number can be converted either to a small integer or to a string.
No other problems were identified. But also this class does not create problems for current extensions, it is completely backward compatible. The changes only apply when implementing types.
But also in databases, there are common data types that are the same in the supported databases. These types work the same everywhere. For special types, you need to create special transformation exceptions. But I looked at 99% of the types, and made them compatible. Types with byte data or interval data, or with coordinate types. PostgreSQL has coordinate types and stuff. This type cannot be universal for other databases, since it is not there. And those that are should work completely.
I hope you will join the testing of this class.
I don't see why there would be B/C issues moving to Doctrine. Leave the old table class alone and introduce Doctrine into future classes as new Entity classes. They can live alongside one another until J5. We shouldn't be trying to solve problems that don't need solving. Doctrine is rock solid and widely known. At the very least we just need the ORM. Storing the ORM data in a giant array makes no sense to me. Reflection is fast so this can just be done with docblocks and later attributes (cache this data on first call and if the file changes rebuild) in J5 with PHP 8 if you insist on using a custom implementation.
I don't think this method of attempting to have typed tables is healthy for the future of Joomla, sorry. This is trying to poorly solve the need for an ORM without just using an ORM.
In development, it is often necessary to check the class for belonging to a JTable. This means that the typed class must inherit from the base JTable. But if there is no need to perform an inheritance check. Then you can use it as a separate class.
The JTable base class does not have type handlers in the methods of callpoints. If we want the Type Table to be inherited, then we need to make small changes to the base class JTable.
I am not familiar with Doctrine. What exactly do you mean by possible functions to support a typed table?
I don't have time for a full adaptation of the Doctrine. Of course, I would really like such full-fledged ORM support.
At the moment I am loaded with current client projects. But I would like to participate in the development of a full-fledged ORM (Doctrines). In the past, I have experience working with LINQ C#.
The below documentation expresses it probably better than I can, lol.
https://www.doctrine-project.org/projects/doctrine-orm/en/2.10/reference/basic-mapping.html
In short Joomla table models need to move away from using dynamic properties and define their properties then we can use Doctrine ORM to define their relationship to the database. This is fine for B/C since Joomla can still use __get/__set for legacy usages while new usages should use typed getter/setter functions. If we don't use Doctrine DBAL then Joomla's DBAL would need to handle using Doctrine ORM functions and checking those annotations, which is probably the only real complicated part but still would be B/C since if those annotations don't exist we'd just behave as we currently do.
Well, the typing of properties in this typed table works, download, install and check. There is no problem here. I've done a lot of tests. And fixed a lot of bugs. It works perfectly everywhere. You can see how the date object property is automatically converted to a date string when data is saved. In this PR, one test has not been passed, I can't catch it in any way. Since this test is performed when testing the API. All other tests have been passed.
This pull request has automatically rebased to 4.2-dev.
This pull request has been automatically rebased to 5.0-dev.
Labels |
Added:
Feature
Conflicting Files
PR-5.0-dev
Removed: ? |
This pull request has been automatically rebased to 5.1-dev.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-11-29 17:41:20 |
Closed_By | ⇒ | rdeutz |
@korenevskiy thanks for working on that issue, there is some need for what you are trying to achive. Code wise I am not a big fan of the implementation. It is really hard to make this change in a b/c way. Further more with the removal of dynamic properties we have some other problems to solve.
So I am closing this, thanks again for you work.
@korenevskiy thanks for working on that issue, there is some need for what you are trying to achive. Code wise I am not a big fan of the implementation. It is really hard to make this change in a b/c way. Further more with the removal of dynamic properties we have some other problems to solve.
So I am closing this, thanks again for you work.
the new table was completely backwards compatible.
What problems do you say still need to be solved?
The class has an array
$_typesVeluesProperty
of matching PHP types and SQL types.This array is the same for different types of databases.
Colleagues, maybe it makes sense to make several arrays for different types of databases?
Example:
$_typesVeluesPropertyMySQL
and$_typesVeluesPropertyPostgreSQL
and$_typesVeluesPropertyNoSQL
?Since different databases may have different preferences for data conversion. But first, let's test with a single array.