Optional FunctionVar Filters in a Dynamic Get #576

Closed
mabdelaziz77 wants to merge 2 commits from master into master
mabdelaziz77 commented 3 years ago (Migrated from github.com)
Owner

Summary of Changes

Referring to this topic: https://groups.google.com/a/vdm.io/forum/#!topic/jcb/b0OUQ80h69Q
I found it is more flexible to make the FunctionVar filters optional, and the case descriped in the above topic explains it in more details.
I just removed the code that adds the else statement in case the filter value is empty.

Testing Instructions

Add a FunctionVar filter to a dynamic get and compile.

Expected result

The generated filter code adds the filter condition only if the filter value is not empty, which makes it optional.

Actual result

The generated filter code has an else statement in case the filter value is empty, it returns false, which causes a database error.

### Summary of Changes Referring to this topic: https://groups.google.com/a/vdm.io/forum/#!topic/jcb/b0OUQ80h69Q I found it is more flexible to make the FunctionVar filters optional, and the case descriped in the above topic explains it in more details. I just removed the code that adds the else statement in case the filter value is empty. ### Testing Instructions Add a FunctionVar filter to a dynamic get and compile. ### Expected result The generated filter code adds the filter condition only if the filter value is not empty, which makes it optional. ### Actual result The generated filter code has an else statement in case the filter value is empty, it returns false, which causes a database error.
Owner

I have read over the communication on the forum, I need to spend more time on this, since it is a change of logic, and current expected behavior. Which will upset upstream users, so making a change the way you did will break many existing components. We will need to be more creative.

I have read over the communication on the forum, I need to spend more time on this, since it is a change of logic, and current expected behavior. Which will upset upstream users, so making a change the way you did will break many existing components. We will need to be more creative.
mabdelaziz77 commented 3 years ago (Migrated from github.com)
Owner

Ok @Llewellynvdm
When you have time, please explain the logic behind this else statement, maybe I can help.

Ok @Llewellynvdm When you have time, please explain the logic behind this else statement, maybe I can help.
Owner

Just that you know, I am not saying that we are not going to make a change, that we should do it in a way that server all. So it is happening.

Just that you know, I am not saying that we are not going to make a change, that we should do it in a way that server all. So it is happening.
Owner

Sorry for the delay here. I have not yet been able to give more time here.

Yet after a quick look at the compiler, it seems like I am treating the filter like an absolute requirement and this serves as a barrier to the data. So that if the filter is not set no data is returned. I understand that is not the expected behavior, but that is what I did, and that is how I have used it in many projects, so have other. So to change this just directly will expose data which otherwise would not have happened. So my idea is to add a switch of some kind that gives the developer the option to allow empty filter values (consciously), and not just change the default (how it works now) and then unconsciously data gets exposed.

If you can come up with some ideas in the meantime that will be helpful, but just removing the else statement without giving the upstream developers a heads-up (switch to choose) is never a good idea.

Sorry for the delay here. I have not yet been able to give more time here. Yet after a quick look at the compiler, it seems like I am treating the filter like an absolute requirement and this serves as a barrier to the data. So that if the filter is not set no data is returned. I understand that is not the expected behavior, but that is what I did, and that is how I have used it in many projects, so have other. So to change this just directly will expose data which otherwise would not have happened. So my idea is to add a switch of some kind that gives the developer the option to allow empty filter values (consciously), and not just change the default (how it works now) and then unconsciously data gets exposed. If you can come up with some ideas in the meantime that will be helpful, but just removing the else statement without giving the upstream developers a heads-up (switch to choose) is never a good idea.
mabdelaziz77 commented 3 years ago (Migrated from github.com)
Owner

Never mind, it is ok.
So, what you suggest and believe that it is more efficient is to add, for example, a checkbox in each filter row, to choose whether the filter to enable/disable having empty filters.

Do you think this is a good implementation?

Never mind, it is ok. So, what you suggest and believe that it is more efficient is to add, for example, a checkbox in each filter row, to choose whether the filter to enable/disable having empty filters. Do you think this is a good implementation?
Owner

Okay the staging branch has moved with almost 100 lines, so merge is not possible anymore.

I have added the checkbox to the filters, that gives the option to allow empty:
image

In the Compiler I have changed the code:
image

image

So the change has been effected in this commit. Let me know if there is any issues.

Okay the staging branch has moved with almost 100 lines, so merge is not possible anymore. I have added the checkbox to the filters, that gives the option to allow empty: ![image](https://user-images.githubusercontent.com/5607939/88484168-0f1a7b00-cf6d-11ea-8e20-31301b42cad9.png) In the Compiler I have changed the code: ![image](https://user-images.githubusercontent.com/5607939/88483722-0ffddd80-cf6a-11ea-84ed-b13357c8246b.png) ![image](https://user-images.githubusercontent.com/5607939/88483738-29068e80-cf6a-11ea-8018-f0d7775592a3.png) So the change has been [effected in this commit](https://github.com/vdm-io/Joomla-Component-Builder/commit/05da68f1ae65a6676fe74415d36a5fbaafc2d47a). Let me know if there is any issues.
mabdelaziz77 commented 3 years ago (Migrated from github.com)
Owner

Great!
Thank you for adding this feature.

Great! Thank you for adding this feature.
This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: joomla/Component-Builder#576
Loading…
There is no content yet.