• Hello Fabrik Community

    Fabrik is now in the hands of the development team that brought you Fabrik for Joomla 4. We have recently transitioned the Fabrik site over to a new server and are busy trying to clean it up. We have upgraded the site to Joomla 4 and are running the latest version of Fabrik 4. We have also upgraded the Xenforo forum software to the latest version. Many of the widgets you might have been used to on the forum are no longer operational, many abandoned by the developers. We hope to bring back some of the important ones as we have time.

    Exciting times to be sure.

    The Fabrik 4.0 Official release is now available. In addition, the Fabrik codebase is now available in a public repository. See the notices about these in the announcements section

    We wish to shout out a very big Thank You to all of you who have made donations. They have really helped. But we can always use more...wink..wink..

    Also a big Thank You to those of you who have been assisting others in the forum. This takes a very big burden off of us as we work on bugs, the website and the future of Fabrik.

PHP 7 delete row failure w/dbjoin repeats

Bauer

Well-Known Member
The onDeleteRows function in datbasejoin.php is not working in php7 - because a variable is being used for an object name without putting it in brackets{}. i.e. $row->$fulName needs to be $row->{$fulName} in php 7.

A pull request with a fix, or revamp of that code, is ready to go - I think.:rolleyes:

I saw no need for creating an array of ids to be deleted from the tablename_repeat_elementname table when what you really should do is just remove any rows that were using the 'parent_id' of the the row that was deleted from the list.

Also there is no need to run a query if there are no options selected. i.e. was executing a query ending with WHERE id IN ("")
 
Note to @cheesegrits - You have a much better idea than I, where else in the Fabrik code such a problem might exist. Unfortunately, I assume there will be at least a few more.:(
 
I've replied on the PR. I suspect your code won't work if more than one row is selected for delete.

-- hugh
 
I've replied on the PR. I suspect your code won't work if more than one row is selected for delete.

-- hugh
Now I get why the loop . OK, I'll test with multiple deletions and fix the code.

So how big a problem do you suspect we have throughout Fabrik - where you have the use of a variable as an object name?
 
Also, I'm pretty sure $foo->$bar syntax is still legal in PHP 7, the curly braces are only required for things like array dereferencing, $foo->{$bar[1]}, or names containing non alphanumeric characters, like xpaths, $foo->{'/a/path?for=example'}.

-- hugh
 
That doesn't mean it won't hurt to harden the code by replacing all $foo->$bar with $foo->{$bar}, partly for readability, but I'm fairly certain it's not a requirement.

A regex search in PHP Storm for "\$(\w+)->\$(\w+)" yields "100+ matches in 26+" files (which means it gave up at 100, and just means "a shitload"). We use that syntax a lot, in a lot of very common code. If it was a systemic issue throwing errors in PHP 7 it would have cropped up long ago.

So I suspect whatever error you were getting was not directly because of the $foo->$bar syntax. What actual error were you getting referencing that line? I just ran a couple of tests, with error reporting on "Development", and no notices, warnings or errors.

-- hugh
 
Also, I'm pretty sure $foo->$bar syntax is still legal in PHP 7, the curly braces are only required for things like array dereferencing, $foo->{$bar[1]}, or names containing non alphanumeric characters, like xpaths, $foo->{'/a/path?for=example'}.

-- hugh
All I can tell you is that without the brackets, I was getting an error.
I'll try to find the stackoverflow.com thread that led me to the solution - and I'll even put the original code back in so you can see the exact php error log verbiage firsthand. I wouldn't have gone through this trouble of fixing it if I hadn't already experienced and 'solved' the problem firsthand.

Do you have a system setup in Fabrik using PHP v7.2x with error log set to 'Development'? (I'm using v.7.29)
If so please try deleting a row in a list that has a multiselect databasejoin element that uses a table_repeat_element file and then look at your php error log - and check the table_repeat_element file to see if the rows got deleted. I'll bet they didn't.

This is one of those examples where just because php doesn't see it as a fatal error, it doesn't mean a "fatal" error hasn't occurred. If an expected database query fails without your knowledge, I'd consider that a 'fatal' error.

Sure this is trivial (orphaned rows in a x_repeat_x table) - but if it's a php 7 issues - and this type of thing is being used in a lot of places in Fabrik, it could be a real headache to find them all.

Or maybe it's just a v7.29 bug and will be fixed in the next php release.
 
As I said ...

I just ran a couple of tests, with error reporting on "Development", and no notices, warnings or errors.

... and it deleted the rows just fine. I'm running 7.2.5. I permanently have error reporting set to Development, so I see all deprecated notices, as well as notice / warning / error.

Also as I said, we use that $foo->$bar syntax in hundreds of places. If this was a change in PHP that doesn't allow $foo->$bar, and insists on $foo->{$bar}, we would be seeing all kinds of errors.

Also, as the current PHP docs I pointed to state:

Class properties may also be accessed using variable property names. The variable property name will be resolved within the scope from which the call is made. For instance, if you have an expression such as $foo->$bar, then the local scope will be examined for $bar and its value will be used as the name of the property of $foo

[...]

Curly braces may also be used, to clearly delimit the property name. They are most useful when accessing values within a property that contains an array, when the property name is made of mulitple parts, or when the property name contains characters that are not otherwise valid (e.g. from json_decode() or SimpleXML).

As far as I know, nothing has changed between 5.x and 7.x to do with when you have to use {} around the object property, apart from some nuanced changes in property access order, which is now strictly left-to-right, and some funky code may need {} to force ordering.

This is why I'm asking exactly what error you were getting, because if it wasn't to do with the {}, then there may still be an issue which needs resolving.

-- hugh
 
here you go @cheesegrits ...
When I deleted one row in the table I got this in the error log - one for each of the dbj elements...

[07-Sep-2018 21:53:14 UTC]warning 'Undefined property: stdClass::$fb_breakouts___filtered_members_raw' at /home/mydomain/public_html/plugins/fabrik_element/databasejoin/databasejoin.php 4030:
[07-Sep-2018 21:53:14 UTC]warning 'Undefined property: stdClass::$fb_breakouts___member_facilities_raw' at /home/mydomain/public_html/plugins/fabrik_element/databasejoin/databasejoin.php 4030:
[07-Sep-2018 21:53:14 UTC]warning 'Undefined property: stdClass::$fb_breakouts___region_raw' at /home/mydomain/public_html/plugins/fabrik_element/databasejoin/databasejoin.php 4030:
[07-Sep-2018 21:53:14 UTC]warning 'Undefined property: stdClass::$fb_breakouts___states_raw' at /home/mydomain/public_html/plugins/fabrik_element/databasejoin/databasejoin.php 4030:
[07-Sep-2018 21:53:14 UTC]warning 'Undefined property: stdClass::$fb_breakouts___homehealth_services_raw' at /home/mydomain/public_html/plugins/fabrik_element/databasejoin/databasejoin.php 4030:

So the correct query for deleting rows from the fb_breakouts_repeat_x table never executes.
 
Last edited:
And also BTW - with the code that was there - where you try to get an array of all the values of the repeat rows (rather than my simpler method of just getting the PK and not have to worry about that) - the results were also inconsistent.

The $rows in $groups - for some reason did not even have any values for many of the databasejoin elements. for example both fb_breakouts___states_raw and fb_breakouts___regions_raw were not in the array of $row values - so they never got processed (after I initially just added the brackets to get past the error). And I have no idea why.

I could not figure were the $groups array that gets passed to this function comes from. Could that, for some reason, be ignoring elements that had been hidden via javascript?

At any rate, I just thought it safer to use the method I used which only needs the PK values form the deleted rows - which would be the 'parent_id' in the repeat table.
 
I can replicate this warning.
But only if the dbjoin element is set to "exclude from list query".
 
I can replicate this warning.
But only if the dbjoin element is set to "exclude from list query".
OK - Now that explains a lot - Thank you, troester. (Have I ever told you that you should work for the CIA? :))

Sure enough - those elements I mention in my last post, that aren't included in the '$groups' array/object, are all marked as 'No' in the 'Include in list query' option for the element.

So that current 'onDeleteRows' code shouldn't be used anyhow - as it is not reliable. The way to delete the repeat data is to use my proposed method Pull Request #2063 posted at Github - Fix onDeleteRows for php 7.

EDIT: I withdrew my pull request. My method would only work on lists with no joined tables.
I suppose I can live with it as is - just need to know that the element must be included in the list query in order to be deleted properly. (I'll add a note to the Wiki)
 
Last edited:
@cheesegrits - I just submitted an amended pull request, to fix the error that was triggered if dbj value was empty (or in $this->getJoin() but not included in the list query).

IMO, when that happens it should at least be noted in the Fabrik log file - if you want to add that as an 'else' in the added 'if' condition, please do.:)
 
That test should probably be ... iff (isset($row->$fullName && !empty($row->$fulName)), as testing empty on an object property that doesn't exist will throw a warning.

And ... I think the real fix for this is to add a check in buildQuery() somewhere to see if we're processing row deletion, and if so ignore "include in query".

That's not to say we shouldn't put the belt and braces test in here, but it doesn't fix the underlying issue of it not working if the element isn't in the query.

I'll take a look at that when I get a minute.

-- hugh
 
That test should probably be ... iff (isset($row->$fullName && !empty($row->$fulName)), as testing empty on an object property that doesn't exist will throw a warning.

And ... I think the real fix for this is to add a check in buildQuery() somewhere to see if we're processing row deletion, and if so ignore "include in query".

That's not to say we shouldn't put the belt and braces test in here, but it doesn't fix the underlying issue of it not working if the element isn't in the query.

I'll take a look at that when I get a minute.

-- hugh
The pre-check in buildQuery is a good idea, if feasible. (Then I could remove the note I added to the Wiki for Elements List view settings Tab.)

But my code changes should still remain (with your correction) - i.e. having both of those 'if' conditions that check for empty values. (e.g. a multiselect or checkbox dbj element with nothing selected - and an empty $keys array).

Else, as I have mentioned, you might end up with a query that ends with WHERE id IN("") - which is a waste of time on a database query, even if it doesn't throw an error.
 
It will throw an error. Pretty much anywhere else in the code we build IN() clauses, we check for empty and avoid it.

Once the site migration has settled down, I'll go take care of it.

-- hugh
 
We are in need of some funding.
More details.

Thank you.

Members online

Back
Top