Thanks @csthomas removed the double null check.

After thinking about this a bit, the only question I have is if we really need both the "getFieldValue" and "getFieldValues" methods. They do quite the same just instead of using a "->where field_id IN (...)" instead of "->where field_id = ...".
That could also be done using the same method with a conditional based on the field_id parameter (multiple ids vs single integer).

Please don't combine the two. That violates SRP.

Stupid question, what is SRP?

single responsibility prinzip

Makes sense ?

So better to have a method to fetch the value(s) for a single field and one for fetching values for multiple fields allthough they do basically the same thing?
If that's preferred, then it is fine as it is. It was just something to consider ?

Right. Basically you want your methods to be responsible for one task and one task only. Fetching the data for a single field and fetching the data for multiple fields are two different tasks, will presumably have two different return types (one might be a value object and the other may be a collection of value objects), and may be post-processed by two different code paths (even if you're doing a foreach loop and it just calls the path for the single item, it still has to have that extra logic). So it's better from a structural standpoint to separate those out.

Changed the getValue function to use getValues and removed the not needed code as pointed out by @joomdonation. Additionally added some inline comments to explain what the function is doing.

@laoneo when you on it could you fix the code style issues

@rdeutz which ones? Travis is running trough.

Ah sorry, didn't realize that drone is now doing the CS stuff. Give me a minute.

Can drone be restarted somehow?

done @laoneo

@rdeutz all good now.

Should we also add here a release blocker label as it reduces the amount of SQL queries?

Added due to the major performance issues this PR aims to address.

I have tested this item ? unsuccessfully on c2af37b

There are SQL error causes by this PR:

1. I created two text fields

2. I created to articles

3. I created a menu item to link to category blog layout menu option

Access to that menu item, see this error

1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') AND item_id = '2'' at line 3

The actual SQL command is

SELECT field_id,value FROM jos_fields_values WHERE field_id IN () AND item_id = '2'

Look like in some condition, field_id doesn't have any data for some reasons

BTW, in my test, look like the custom fields are not being displayed for Category Blog menu option. So in that case, could we stop querying the database to get data? (I haven't tested this feature much so far, so maybe I just mis-understand how it works)

I don't think I would hardcode the plugin context to exclude category views. In theory someone could do a category layout using fields if desired.

OK. That makes sense (although with default usage like this, it is a waste of resource to query data without using it - It might has performance impact). Also, one thing about category view which I cannot test right now because above error. Before patch, for each item, an SQL query is is generated to get data for a field, so with 2 articles, 2 fields, 4 query total. I guess if this patch works, there will only one query. Do we want to merge it to one query to get data for all custom fields of all articles (not sure if it is possible)

Do we want to merge it to one query to get data for all custom fields of all articles (not sure if it is possible)

Not possible with the current plugin events. You would need a new event for the whole list.

@joomdonation I'v added a test if the field ids are empty. Now the error should be gone.

I have tested this item ✅ successfully on 23fac40

OK. It works now. Unrelated to this PR, on category blog page, still one query per article if you want to improve (in case it can be improved)

Open an issue for it. But as @Bakual said, it needs a new event for it, probably not a bad idea, but I'm not sure if it is doable in the 3 series.

I have tested this item ✅ successfully on 23fac40

with 100 items in list number of queries is reduced from 1000 to 100, there is also jsut a single query on single item.

This is much better but still not quite there (not the fault of this PR), we have the same problem with tags.

Guess RTC?

work now with Falang

Please all test again - I think this broke something in the current staging after this was merged

#14932