Icinga / icingaweb2-module-fileshipper

Provide CSV, JSON, XML and YAML files as an Import Source for the Icinga Director and optionally ship hand-crafted additional Icinga2 config files
https://icinga.com/docs/director/latest/fileshipper/doc/02-Installation/
GNU General Public License v2.0
29 stars 13 forks source link

Refactor listColumns() method to remove deprecated current() function #41

Closed martialblog closed 11 months ago

martialblog commented 11 months ago

https://www.php.net/manual/en/function.current.php

martialblog commented 11 months ago

@Thomas-Gelf Tried to handle this like other ImportSources in the Director.

Wasn't sure how to handle empty data. Let me know if there's a better way.

Thomas-Gelf commented 11 months ago

@martialblog: thanks you! IMHO the bug is elsewhere, fetchData() should return an Array of objects:

    /**
     * Returns an array containing importable objects
     *
     * @return array
     */
    abstract public function fetchData();

So, if fetchData ships an object instead, fetchData() should be fixed and return (array) $whatever.

martialblog commented 11 months ago

fetchData() already returns an array as expected return (array) $this->fetchFile

I meant how the listColumns should handle empty data from fetchData(). ImportSourceHook defines it as such, so I assumed an empty array would be fine.

    /**
     * Returns a list of all available columns
     *
     * @return array
     */
    abstract public function listColumns();
martialblog commented 11 months ago

I just realized the on objects part in Calling this function on objects is deprecated. So calling it on arrays is fine, right?

Thomas-Gelf commented 11 months ago

Right, and empty data is an empty array. So this should never trigger a related notice. Checking for an empty result is still useful, as (array) false evaluates to [false], which would be presented as a single column named '' (emtpy string). Returning no column at all is better. However, it makes no difference in practice, as you're not able to store a sync property referencing an empty string