Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TMultiList and TMultiMap for accessing multi dimensional arrays. #958

Open
belisoful opened this issue May 19, 2023 · 8 comments
Open

TMultiList and TMultiMap for accessing multi dimensional arrays. #958

belisoful opened this issue May 19, 2023 · 8 comments

Comments

@belisoful
Copy link
Member

belisoful commented May 19, 2023

I'm reviewing Yii2 BaseArrayHelper and comparing it to what we have. Their BaseArrayHelper is basically a replacement for TMap. It adds a few things that we don't do, like various ways of ordering, indexing, and organizing arrays, merging, sorting, determining the type of array (associative or indexed), and some html functions. However, there are some things we can learn and do better.

  • TMap could use these functions:
	public function removeItem($item): null|array
	{
		$return = [];
		foreach ($this->toArray() as $key => $value) {
			if ($item === $value) {
				$return[$key] = $this->remove($key);
			}
		}
		return $return;
	}
	public function itemAtIndex($index): null|mixed
	{
		return array_values($this->toArray())[$index] ?? null;
	}

// maybe these introspective methods as well.  these use a common modality.  Even if custom made by hand, the code would look like Yii2 (that's what I mean by common modality).
public function getIsAssociative (bool $isAllString = true):bool;
public function getIsIndexed(bool $consecutive = false):bool;
  • TList::removeAllItem to remove all of an item, to match TMap::removeItem. an array of indices removed is returned.
    or maybe remove should remove all of an item rather than just the first instance, that would be more consistent and parallel yii2.
    @ctrlaltca Do you have any insight into what to do here? should we update the remove function or add a new removeAllItem function?
    If we update the existing, it would return false if not found, the index if found, and an array if multiple are found. I like updating the existing function to remove all an item rather than having a second function. It's more consistent with expectation.

  • TMap and TList could be updated such that their itemAt, add/insertAt, remove[At], indexOf accepts "." for traversal of the array and returning, setting, and unsetting of a sub-array or item-object property. keyOf and indexOf could, upon an optional parameter, become recursive and search sub-arrays, returning the path. This is something I am looking at as a function of TEXIF to traverse its tree and traversal of JPEG metadata (IPTC, EXIF, XMP) accessibly via "EXIF.GPSInfo.GPSLatitude". This would be easier if it was already built into the TList/TMap class.

The TList and TMap index/key could act like this "0.10.mapkeyitem" to get, and set an item in the structure. and "0.10.mapkeyitem.propertyA" to get and set internal properties.
@ctrlaltca Any thoughts on implications of this functionality in general? Do you like the idea? good? bad? important? low value? does it add any value? what about "competing" with yii[2/3]?

$map->add('0.10.mapkey.propertya', $value) may seem odd instead of $map[0][10]['mapkey']->setPropertyA($value), but this is the intended use case:
$list['0.10.mapkey.propertya'] = $value;

I suggest that when adding, a special end index "NULL" be used for inserting at the end of the map/list.
In this way, $map['key'][] = $value becomes $map['key.NULL'] = $value

While this could be implemented as a subclass of TMap and TList, like TMapTree and TListTree, my thinking is that adding this to the core would be beneficial in the long run. Having these functions on a basic array is awesome and why I was looking at it to begin with.

there needs to be regression and unit tests for this as well, of course.

This is a low priority enhancement.

@ctrlaltca
Copy link
Member

TMap currently is a key based unordered map, backed by a php array. As such, keys are unique but values can be duplicated.
Trying to sum up the proposed improvements:

  • a new removeItem()/removeAllItems() method to remove an item by value (currently we can only remove() by key). Since values can be duplicated, this can actually remove more than one item per call, so it returns a (possibly empty) array of removed key=>values.
    Adding a new item looks fine to me. Modifying the current remove() doesn't seems a viable solution, since it operates on keys instead of values.
  • a new itemAtIndex() method to get a value by its index in the array.
    Since TMap is not ordered, basically this would return items in order of insertion.
    I don't see a real use on this, maybe to use TMap as a stack / queue?
  • make TMap multi-dimensionally traversable adding a 'dot' notation on keys
    This is potentially breaking for backwards compatibility; some places in Prado itself uses TMap to save collection of properties (eg. TApplication:_parameters), and they can already contain dots in the key, that would be interpreted in a different way.
    This scares me,but maybe i'm mistaken. I'd prefer to see a new class

@belisoful
Copy link
Member Author

belisoful commented May 19, 2023

Thank you for your insight, esp the last point.

some clarification and subtlety:

  • TList can have one item in multiple indices, yes? remove only removes the first. should it remove all of them? Is this the proper behavior? if removing multiple would [n => $item, m => $item] be the proper output?
  • TMap removeItem should [] if nothing was removed, yes
  • I was thinking of if a subclass reorders TMap then itemAtIndex may be useful. that is specific to the subclass, you are right about that. Scrub this one. just an imagination. do we imagine like AI does? lol

@belisoful
Copy link
Member Author

belisoful commented May 19, 2023

Also, the way PNG does multidimensionality is with a ":" character instead, which would be both more compatible (likely) and more confusing. I think making the character a choice of the subclass would be ideal. This way a TPNGFileMetaData type class could override the '.' and use the ':' character instead.

Application::Paramete might be able to use the new TMap multidimensionality.

@belisoful
Copy link
Member Author

It is looking like the multidimensional access should be its own TMultiMap class and TMultiList class.

@belisoful
Copy link
Member Author

@ctrlaltca Is TList::remove only removing one Item, rather than all of the item, the proper result of the function?

obviously if one item is found, only return it's index and remove (backward compatibility), but multiple item? maybe remove should operate on all placements of an item in the list and return the [key => item, key2 => item] if multiple are found?

@belisoful
Copy link
Member Author

belisoful commented May 21, 2023

I have an interesting idea.... Instead of multidimensional functionality be built into TList and TMap with a switch (default off? eh?), or new classes TMultiMap/TMultiList, What if it were a behavior to attach to TMap and TList?

Prado\Collections\TMultiDimensionalBehavior

It should work on all TList and TMap, so some regression would be needed for the more advanced Collection classes.

This would continue the integration of behaviors into Collections.

  • I do like the idea of building multidimensionality into TList and TMap with a switch that is default false. That should resolve your sense of potentially breaking things. Making it more direct reduces the overhead of dynamic events.
  • I also like the idea of it being a behavior. The behavior itself takes all the multidimensional behavior out of the collection itself and introduces a few dynamic events and conditions to the core collection classes. dynamic events are designed to be quick and wispy though it has a little bit of overhead... and most collections won't ever use it
  • for simplicity, I see why a TMultiMap or TMultiList might be preferable.

I'm still doing research into which modality would be "best." If you have input, please, your guidance has been of the greatest contribution each time.

<./ important >

Just FYI, The reason for multidimensional array capability is for accessing image metadata. I'd like to get this functional:

$author = $imageMeta['IPTC:ByLine'];
$imageMeta['IPTC:1#90'] = 'UTF-8';
$latitude = $imageWebPMeta['EXIF:GPSInfo:GPSLatitude'];
$imagePNGMeta['XMP:Keyword:NULL'] = 'new appended keyword';
$numKeywords = $imagePNGMeta['XMP:Keyword:count()'] // special keyword for counting arrays?
$imageJPEGMeta['EXIF:IPTCNAA'] = $imageMeta['IPTC'];  //. <- IPTC can be stored in EXIF, making things that much more complex.

$imageWebPMeta['EXIF']['GPSInfo']['GPSLatitude'] === $imageWebPMeta['EXIF:GPSInfo:GPSLatitude'];

While a chain of PHP array access is ok, having string access to arrays and properties opens up uniformity and configurability to accessing multi-dimensional array data in a more user friendly and configuration friendly way.

PHPs support is extremely poor for creating, read, updating, deleting, and saving metadata and metadata within images. for example, PHP's getImageSize returns the EXIF data from a JPEG but if XMP is saved in the same tag, it is not retrievable. EXIF overwrites the XMP as only one metadata app tag is "enabled" for getImageSize metadata. Once this data is retrieved (or not, in the case of EXIF overloading XMP in a double APP1 circumstance), there is no function to write new metadata to a JPEG.

It is not clear what formats getImageSize supports. clearly JPEG. but webp meta? and/or PNG meta? it could be tested, but it should already be clear. jpeg only? iptcparse is crap and there is no method to go back from its outputted array back to binary for saving. The "open source" comment is good enough but needs to be better, proofed and tested.

So JPEG needs a better read/write metadata functions. but then so does PNG and WebP... and if IPTC is getting better support, in comes EXIF and XMP. really good support for image metadata, I envision, is part of the new asset manager.

JPEG, PNG, and WebP should have a Multi-dimensional array access of their respective metadata for configuration. I envision image filters selecting on meta data properties coming down the line.

@belisoful
Copy link
Member Author

I'm leaning towards a TMultiMap and TMultiList again after imagineering other ideas.

@belisoful belisoful changed the title RFC: TList/TMap adds "." operator to key/index for accessing subarrays and sub object-properties TMultiList and TMultiMap for accessing multi dimensional arrays. May 21, 2023
@belisoful
Copy link
Member Author

In working on Image file metadata reading and updating and the meta data (iptc, exif, xmp), I found a bug in PHPStan.

I think that's some kind of GitHub badge of something. maybe an honorary badge of "too much work"?
The issue report:
phpstan/phpstan#9341

The bug
https://phpstan.org/r/7304abb9-505e-40e3-b8cc-1d340f4e802b

belisoful added a commit to belisoful/prado that referenced this issue May 23, 2023
Removes all of an item from the map
ctrlaltca pushed a commit that referenced this issue May 25, 2023
* #958 TMap::removeItem

Removes all of an item from the map

* is this the reason?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants