Don't store unknown/invalid fields in Extra in non-strict mode

And fix a couple things for if we turn it back on

This code came along with the type/field handling overhaul, but I think
it was originally intended for handling unknown fields during sync
before we decided on strict mode, so it wasn't finished and causes
various problems [1]. It could still be useful for preserving fields
from translators before they're available on items, but the better fix
there is just to add the missing fields, so I'm not sure if we'll end up
needing it.

[1] https://groups.google.com/d/msg/zotero-dev/a1IPUJ2m_3s/hfmdK2P3BwAJ

chrome/content/zotero/xpcom/data/item.js

@@ -4206,11 +4206,11 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) {
 	
 	var isValidForType = {};
 	var setFields = new Set();
-	var { fields: extraFields, extra } = Zotero.Utilities.Internal.extractExtraFields(
+	/*var { fields: extraFields, creators: extraCreators, extra } = Zotero.Utilities.Internal.extractExtraFields(
 		json.extra !== undefined ? json.extra : '',
 		this,
 		Object.keys(json)
-	);
+	);*/
 	
 	// Transfer valid fields from Extra to regular fields
 	// Currently disabled
@@ -4266,6 +4266,7 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) {
 			break;
 		
 		case 'creators':
+			//this.setCreators(json.creators.concat(extraCreators), options);
 			this.setCreators(json.creators, options);
 			break;
 		
@@ -4316,11 +4317,12 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) {
 					throw e;
 				}
 				// Otherwise store in Extra
-				if (typeof val == 'string') {
+				// TEMP: Disabled for now, along with tests in itemTest.js
+				/*if (typeof val == 'string') {
 					Zotero.warn(`Storing unknown field '${field}' in Extra for item ${this.libraryKey}`);
 					extraFields.set(field, val);
 					break;
-				}
+				}*/
 				Zotero.warn(`Discarding unknown JSON ${typeof val} '${field}' for item ${this.libraryKey}`);
 				continue;
 			}
@@ -4342,9 +4344,12 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) {
 					throw e;
 				}
 				// Otherwise store in Extra
-				Zotero.warn(`Storing invalid field '${origField}' for type ${type} in Extra for `
+				// TEMP: Disabled for now, since imports can assign values to multiple versions of
+				// fields
+				// https://groups.google.com/d/msg/zotero-dev/a1IPUJ2m_3s/hfmdK2P3BwAJ
+				/*Zotero.warn(`Storing invalid field '${origField}' for type ${type} in Extra for `
 					+ `item ${this.libraryKey}`);
-				extraFields.set(field, val);
+				extraFields.set(field, val);*/
 				continue;
 			}
 			this.setField(field, json[origField]);
@@ -4352,7 +4357,8 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) {
 		}
 	}
 	
-	this.setField('extra', Zotero.Utilities.Internal.combineExtraFields(extra, extraFields));
+	//this.setField('extra', Zotero.Utilities.Internal.combineExtraFields(extra, extraFields));
+	this.setField('extra', json.extra !== undefined ? json.extra : '');
 	
 	if (json.collections || this._collections.length) {
 		this.setCollections(json.collections);

chrome/content/zotero/xpcom/utilities_internal.js

@@ -993,7 +993,7 @@ Zotero.Utilities.Internal = {
 		var skipKeys = new Set();
 		var lines = extra.split(/\n/g);
 		for (let line of lines) {
-			let parts = line.match(/^([a-z -_]+):(.+)/i);
+			let parts = line.match(/^([a-z][a-z -_]+):(.+)/i);
 			// Old citeproc.js cheater syntax;
 			if (!parts) {
 				parts = line.match(/^{:([a-z -_]+):(.+)}/i);

test/tests/itemTest.js

@@ -1799,7 +1799,7 @@ describe("Zotero.Item", function () {
 			assert.equal(item.getField('extra'), `doi: ${doi2}`);
 		});*/
 		
-		it("should store unknown field in Extra in non-strict mode", function () {
+		it.skip("should store unknown field in Extra in non-strict mode", function () {
 			var json = {
 				itemType: "journalArticle",
 				title: "Test",
@@ -1811,7 +1811,7 @@ describe("Zotero.Item", function () {
 			assert.equal(item.getField('extra'), 'foo: Bar');
 		});
 		
-		it("should replace unknown field in Extra in non-strict mode", function () {
+		it.skip("should replace unknown field in Extra in non-strict mode", function () {
 			var json = {
 				itemType: "journalArticle",
 				title: "Test",

test/tests/utilities_internalTest.js

@@ -190,6 +190,12 @@ describe("Zotero.Utilities.Internal", function () {
 			assert.equal(extra, "Publisher Place: " + place2);
 		});
 		
+		it("shouldn't extract a field from a line that begins with a whitespace", function () {
+			var str = '\n number-of-pages: 11';
+			var { fields, extra } = Zotero.Utilities.Internal.extractExtraFields(str);
+			assert.equal(fields.size, 0);
+		});
+		
 		it("shouldn't extract a field that already exists on the item", function () {
 			var item = createUnsavedDataObject('item', { itemType: 'book' });
 			item.setField('numPages', 10);
@@ -198,6 +204,17 @@ describe("Zotero.Utilities.Internal", function () {
 			assert.equal(fields.size, 0);
 		});
 		
+		it("should extract an author and add it to existing creators", function () {
+			var item = createUnsavedDataObject('item', { itemType: 'book' });
+			item.setCreator(0, { creatorType: 'author', name: 'Foo' });
+			var str = 'author: Bar';
+			var { fields, creators, extra } = Zotero.Utilities.Internal.extractExtraFields(str, item);
+			assert.equal(fields.size, 0);
+			assert.lengthOf(creators, 1);
+			assert.equal(creators[0].creatorType, 'author');
+			assert.equal(creators[0].name, 'Bar');
+		});
+		
 		it("should extract a CSL name", function () {
 			var str = 'container-author: First || Last';
 			var { creators, extra } = Zotero.Utilities.Internal.extractExtraFields(str);