Items that have beend defined by material+data value (example colored wool) in the past, are not getting converted correctly in various situations.

I have tested this for the case of bukkit's ItemStack serialization, but this should also affect any items that get created via new ItemStack(type, amount, dataValue) and any items created via the MaterialData classes.

Edit: I did some more testing: It seems that the items get 'lazily' converted once they are converted to a corresponding nms ItemStack (see CraftItemStack#asNMSCopy). I have update the below example accodingly. However, this lazy conversion doesn't work for deserialized items. I will outline what I think is the cause of the issue and possible solutions at the end of this ticket.

Test plugin:

package de.blablubbabc.test2;

import org.bukkit.Bukkit;
import org.bukkit.Material;
import org.bukkit.event.EventHandler;
import org.bukkit.event.Listener;
import org.bukkit.event.player.PlayerJoinEvent;
import org.bukkit.inventory.ItemStack;
import org.bukkit.plugin.java.JavaPlugin;

public class Test extends JavaPlugin implements Listener {

	public static Test INSTANCE = null;

	@Override
	public void onEnable() {
		INSTANCE = this;
		Bukkit.getPluginManager().registerEvents(this, this);

		if (Bukkit.getVersion().contains("1.12")) {
			this.getLogger().info("Saving ..");
			ItemStack saved = new ItemStack(Material.WOOL, 1, (byte) 14);
			this.getLogger().info("" + saved);
			this.getConfig().set("test", saved);
			this.saveConfig();
		} else {
			this.getLogger().info("Loading ..");
			ItemStack loaded = this.getConfig().getItemStack("test");
			this.getLogger().info("" + loaded);
			this.getLogger().info("Saving loaded itemstack..");
			this.getConfig().set("test", loaded);
			this.saveConfig();
		}
	}

	@Override
	public void onDisable() {
		INSTANCE = null;
	}

	@EventHandler
	void onPlayerJoin(PlayerJoinEvent event) {
		Bukkit.getScheduler().runTaskLater(this, () -> {
			event.getPlayer().getInventory().addItem(new ItemStack(Material.WOOL, 1, DyeColor.BLUE.getWoolData())); // works
			event.getPlayer().getInventory().addItem(new Wool(DyeColor.PINK).toItemStack(1)); // works
			event.getPlayer().getInventory().addItem(this.getConfig().getItemStack("test"));
		}, 1L);
	}
}

name: Test
version: '1.0'
description: Testing some stuff
main: de.blablubbabc.test2.Test

This is meant to simulate a plugin that uses bukkit's ItemStack serialization to save and load items. To simulate the updating to 1.13, this first gets run on a 1.12.2 spigot server: There it will create and save a red wool item. Any other items that have been defined via data value and split into separate items in 1.13 should be affected by this as well.
The saved config looks like this:

test:
  ==: org.bukkit.inventory.ItemStack
  type: WOOL
  damage: 14

And a joining player will receive a red wool item as expected.

Now consider the server being updated to 1.13. The plugin gets run as is (assuming forward compatibility) and attempts to load the previously saved red wool ItemStack from the config.
To visualize the outcome, the item gets saved back to the config, which looks like this then:

test:
  ==: org.bukkit.inventory.ItemStack
  v: 1513
  type: WHITE_WOOL
  meta:
    ==: ItemMeta
    meta-type: UNSPECIFIC
    Damage: 14

And the item given to a joining player will be a white wool item.

Expected behavior: The ItemStack should probably get converted at some point to a RED_WOOL item.

Currently it seems to interpret those legacy data values as damage values (even though the item is not considered damageable in MC 1.13), and not convert them.

Related to this, but more of a 'feature request':
Currently it seems to only be possible to convert legacy materials via Material#get/matchMaterial(material, true). Is there any possibility for a plugin to convert the combination of material+dataValue (basically the old MaterialData) to a corresponding 1.13 material?

Edit: Possible cause of the issue:
When an old ItemStack is deserialized, its material will instantly get converted to a new material by Material.getMaterial([..]) (because no version ('v') is present in the serialized data) (see ItemStack#deserialize). I assume this might convert the legacy wool material to a modern white_wool material. Once the item is then converted to a nms itemstack, it performs another conversion which now takes both the material and data/damage value into account: However, this conversion will probably not work, because the material already got converted (it's no longer a legacy wool at that point).

So possible solutions might be:

• Don't convert the legacy item during deserialization. The ItemStack will internally keep using the legacy material (similar to how it's the case for items created in old plugins via Wool MaterialData or via 'new ItemStack(Material.Wool, 1, dataValue)'), and only convert it lazily when being converted to an nms item. In this case plugins will have to manually convert their loaded legacy ItemStacks via Bukkit.getUnsafe().fromLegacy(MaterialData).
• Don't convert lazily, but do the material type conversion directly inside the ItemStack (for example when setDamage is called, or as part of CraftItemMeta updateMaterial?). Converting the materials automatically and as early as possible will have the advantage that updated plugin's will only have to see and use the modern materials when being handed a loaded itemstack or an itemstack that was created by an older plugin, and when serialized again it will use the modern material. Otherwise those ItemStack will linger around internally inside the API parts with their legacy materials and data values.
  However, if I think about this, this might also break older plugins that expect the item to be of a certain type and damage/data value..
• Alternative 3: Keep everything as is (lazy conversion for most parts), but take the damage value into account when converting the material of an ItemStack during deserialization.
  I don't have the time to prepare and test a PR right now, but something similar in behavior to this (completely untested): http://prntscr.com/k7j71u

Edit2: Actually, if I think about this a bit more, and if I understand the effect of the bytecode conversion stuff correctly, there might still be potential of old plugins breaking with this, if deserialized itemstacks get converted..:
If an older plugin (using legacy materials in its item type comparisons) loads, saves and loads an ItemStack, and the ItemStack gets converted by this to its new material, do the plugin's checks of type 'if (loadedItem.getType() == Material.WOOL) [..]', with WOOL being replaced by LEGACY_WOOL automatically, still keep working, or break?
If not, then the first proposed solution (not automatically converting ItemStacks during deserialization and leaving it to the plugins update handling..) might be preferred. (Basically something like this: http://prntscr.com/k7jmao)

Bug reports which do not 1) contain a declaration of testing in Vanilla and without plugins, or 2) in the case of plugin API bugs, contain a minimal reproduction case (source + jar please) will be closed. Bug reports must contain step by step instructions to reproduce the bug from a clean server install with no assumptions or prior knowledge. Also make sure you include the full output of /version in your report. Please copy and paste this statement to the bottom of your report to indicate that you have read and understood it.