Files: 
CommentFileSizeAuthor
#38 1987140-38.patch10.79 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,829 pass(es).
[ View ]
#34 1987140-34.patch10.72 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,831 pass(es).
[ View ]
#34 interdiff-1987140-34.txt830 bytesdamiankloip
#32 1987140-32.patch10.7 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 56,626 pass(es).
[ View ]
#30 1987140-29.patch11.1 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#30 interdiff-1987140-29.txt3.1 KBdamiankloip
#27 1987140-27.patch12.05 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,926 pass(es).
[ View ]
#27 interdiff-1987140-27.txt413 bytesdamiankloip
#25 1987140-25.patch12.46 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#21 1987140-21.patch12.44 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#21 interdiff-1987140-21.txt9.87 KBdamiankloip
#14 1987140-14.patch7.01 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,581 pass(es).
[ View ]
#14 interdiff-1987140-14.txt1.27 KBdamiankloip
#11 1987140-11.patch5.74 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 55,631 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#6 1987140-6.patch5.67 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,663 pass(es).
[ View ]
#6 interdiff-1987140-6.txt840 bytesdamiankloip
edit-editor-annotation.patch5.08 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php.
[ View ]

Comments

damiankloip’s picture

Assigned:Unassigned» damiankloip
damiankloip’s picture

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, edit-editor-annotation.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review

edit-editor-annotation.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, edit-editor-annotation.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new840 bytes
new5.67 KB
PASSED: [[SimpleTest]]: [MySQL] 55,663 pass(es).
[ View ]

Editor name conflict. Maybe the editor modules Editor class can be PropertyEditor instead (based on the class description)?

Wim Leers’s picture

Better: InPlaceEditor.

Also, why do we have namespaces if we can't leverage that anyway? One of the big points of namespaces is precisely that you shouldn't have to worry about classes with the same name… but my guess is that the plugin system doesn't support namespaces?

Wim Leers’s picture

Upon further consideration — I don't think I understand what name conflict there is?

dawehner’s picture

Well, the problem is that you can't have the same classname twice in a context of a class. You can't change the "active" class (as that's what you add in the file) so the other class (the annotation) need to be different.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/PropertyEditor.phpundefined
@@ -16,14 +16,14 @@
  *   id = "editor",
  *   jsClassName = "editor",

maybe we should rename the id as well?

Wim Leers’s picture

Sorry, I didn't see you already had done that rename.

I meant that Edit's "Editors" in general could potentially be renamed to "InPlaceEditors".

But this:

Maybe the editor modules Editor class can be PropertyEditor instead (based on the class description)?

makes no sense, unfortunately.

I now also understand what you were saying in #6: you have to use Drupal\edit\Annotation\Editor;, but the class in which this is used is already called Editor :) Now THAT makes sense!
The solution is simple though: use My\Full\Classname as Another;, i.e. use Drupal\edit\Annotation\Editor as InPlaceEditor. Or doesn't the whole "plugin system annotation thing" allow for aliasing @Editor to @InPlaceEditor?

If that's the case, then I'd rename \Drupal\edit\Annotation\Editor to \Drupal\edit\Annotation\InPlaceEditor. The funny thing is that I originally advocated for something like that, but reviewers said something like "we have namespaces, so why would we want to include qualifiers that we don't need — let's just call it 'editor'"…

damiankloip’s picture

StatusFileSize
new5.74 KB
FAILED: [[SimpleTest]]: [MySQL] 55,631 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

The solution is simple though: use My\Full\Classname as Another;, i.e. use Drupal\edit\Annotation\Editor as InPlaceEditor

Yeah, you can't do that. So not simple, unfortunately.. If we use as InPlaceEditor then the annotation will have to be @inPlaceEditor, which then breaks the whole consistency of having plugin specific annotations.

Your comment in #10 contradicts itself slightly :) Would you prefer the annotation to be renamed to InPlaceEditor or the actual Editor class that I renamed to PropertyEditor (which was kind of a guess)?

Here is a patch that renames the Editor plugin class to InPlaceEditor, as this seems like the best choice to me. I'm happy to reroll if you disagree though.

Status:Needs review» Needs work

The last submitted patch, 1987140-11.patch, failed testing.

tim.plunkett’s picture

Issue tags:+Plugin system, +Annotation

Tagging.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new1.27 KB
new7.01 KB
PASSED: [[SimpleTest]]: [MySQL] 55,581 pass(es).
[ View ]

Whoooops.

Status:Needs review» Needs work
Issue tags:-Plugin system, -Annotation

The last submitted patch, 1987140-14.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
Issue tags:+Plugin system, +Annotation

#14: 1987140-14.patch queued for re-testing.

Doesn't seem related ...

dawehner’s picture

+++ b/core/modules/edit/tests/modules/lib/Drupal/edit_test/Plugin/edit/editor/WysiwygEditor.phpundefined
@@ -8,13 +8,13 @@
  *   jsClassName = "not needed for test",
  *   alternativeTo = {"direct"},

Just in general it feels wrong to have camel-case names for this properties.

dawehner’s picture

quicksketch’s picture

Title:Add a dedicated @Editor plugin annotation» Add a dedicated @InPlaceEditor plugin annotation
Status:Needs review» Needs work

Hey guys, we absolutely should not be using the Editor namespace here. We need that for the editor.module's declaration of editor plugins such as CKEditor.

There wasn't a separate issue for it yet, so I opened it at #1992744: Add a dedicated @Editor plugin annotation.

For this issue I'd be happy to see InPlaceEditor be the defined namespace, and then work on #1874640: Rename edit module to quickedit as a follow-up.

damiankloip’s picture

Sounds fine, thanks for the clarification. Anything that gets us out of naming hell between these modules :)

I'll reroll.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new9.87 KB
new12.44 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

So something like this?

Status:Needs review» Needs work

The last submitted patch, 1987140-21.patch, failed testing.

quicksketch’s picture

new file mode 100644
index 0000000..f3ea3e6
--- /dev/null
+++ b/core/modules/edit/lib/Drupal/edit/Annotation/Editor.php

This should be InPlaceEditor.php

diff --git a/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php b/core/modules/editor/lib/Drupal/editor/Plugin/InPlaceEditor/Editor.php
similarity index 92%
rename from core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
rename to core/modules/editor/lib/Drupal/editor/Plugin/InPlaceEditor/Editor.php
index 81c23d7..d1eb344 100644
--- a/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
+++ b/core/modules/editor/lib/Drupal/editor/Plugin/InPlaceEditor/Editor.php

/**
- * Defines the "editor" Create.js PropertyEditor widget.
+ * Defines the "editor" Create.js InPlaceEditor widget.
  *
- * @Plugin(
- *   id = "editor",
+ * @InPlaceEditor(
+ *   id = "in_place_editor",
  *   jsClassName = "editor",

To further help the namespacing problems we've got with Edit and Editor module, would it be sensible to rename this class to something like Default.php? So it would be core/modules/editor/lib/Drupal/editor/Plugin/InPlaceEditor/Default.php? I mean, I doubt there are going to be too many alternative in-place editors written, but still, the phrase "default in-place editor" makes a lot more sense than "the editor in-place editor".

Sorry I thought this plugin was provided by Edit module. It makes sense as-is. Of course the Editor module should be responsible for the Editor in-place editor. Unfortunately named yes, but it makes sense.

quicksketch’s picture

Oh geez, I just realized that in #1886566: Make WYSIWYG editors available for in-place editing, we created a property called "supports_inline_editing". What a mess. I suppose the followup we can name this to "supports_in_place_editing"? I really wish "inplace" where just a single word, or that "inline" was simply acceptable to everyone. Any opinions on this @damiankloip?

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new12.46 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

This should be InPlaceEditor.php

Hehe, I knew I missed something silly.

...would it be sensible to rename this class to something like Default.php

Just for future reference, default is a reserved word in php (i.e. in a switch), so you can't have classes named 'Default' unfortunately.

Hmm, it's tricky. I would say that in place is a much better term for describing what it is, there is less ambiguity IMO. However, I also agree that inline is good because it is a single word - I'm not sure inplace works unfortunately. Otherwise I would agree it would be the way to go. So, yes, I think there should be a follow up to change use of inline.

Here is a reroll with the file rename.

Status:Needs review» Needs work

The last submitted patch, 1987140-25.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new413 bytes
new12.05 KB
PASSED: [[SimpleTest]]: [MySQL] 55,926 pass(es).
[ View ]

Over zealous on the manager name change...

Wim Leers’s picture

Issue tags:+sprint, +Spark

#1979784: Factor Create.js and VIE.js out of the Edit module got committed yesterday, which means #27 will no longer apply.

+++ b/core/modules/edit/lib/Drupal/edit/Annotation/InPlaceEditor.phpundefined
@@ -0,0 +1,48 @@
+ * Defines an Editor annotation object.

s/Editor/InPlaceEditor/

+++ b/core/modules/edit/lib/Drupal/edit/Annotation/InPlaceEditor.phpundefined
@@ -0,0 +1,48 @@
+   * The Javascript class name associated with this plugin.

This is gone as of #1979784: Factor Create.js and VIE.js out of the Edit module.

+++ b/core/modules/edit/lib/Drupal/edit/Annotation/InPlaceEditor.phpundefined
@@ -0,0 +1,48 @@
+   * An array of editors that have registered themselves as alternatives to this

s/editor/in-place editor/

+++ b/core/modules/editor/lib/Drupal/editor/Tests/EditIntegrationTest.phpundefined
@@ -133,7 +133,7 @@ function testEditorSelection() {
-    $this->assertEqual('editor', $this->getSelectedEditor($items, $this->field_name), "With cardinality 1, and the full_html text format, the 'editor' editor is selected.");
+    $this->assertEqual('in_place_editor', $this->getSelectedEditor($items, $this->field_name), "With cardinality 1, and the full_html text format, the 'editor' editor is selected.");

Undo this change, see next comment.

+++ b/core/modules/editor/lib/Drupal/editor/Tests/EditIntegrationTest.phpundefined
@@ -164,7 +164,7 @@ function testMetadata() {
-      'editor' => 'editor',
+      'editor' => 'in_place_editor',

No, this should be 'editor', not 'in_place_editor'. It refers to editor.module's in-place editor.

Wim Leers’s picture

Status:Needs review» Needs work

.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new3.1 KB
new11.1 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Tired reroll...

Status:Needs review» Needs work

The last submitted patch, 1987140-29.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new10.7 KB
PASSED: [[SimpleTest]]: [MySQL] 56,626 pass(es).
[ View ]
Wim Leers’s picture

Status:Needs review» Needs work

Down to mere nitpicks, then this'll be RTBC. Sorry :/

+++ b/core/modules/edit/lib/Drupal/edit/Annotation/InPlaceEditor.phpundefined
@@ -0,0 +1,41 @@
+   * An array of in-place-editors that have registered themselves as

s/in-place-editors/in-place editors/

… or is that actually against English grammar? I'm not a native speaker, you are AFAICT :)

+++ b/core/modules/edit/lib/Drupal/edit/Annotation/InPlaceEditor.phpundefined
@@ -0,0 +1,41 @@
+   * alternatives to this editor.

s/editor/in-place editor/

+++ b/core/modules/edit/lib/Drupal/edit/Annotation/InPlaceEditor.phpundefined
@@ -0,0 +1,41 @@
+   * The name of the module providing the editor plugin.

s/editor/in-place editor/

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new830 bytes
new10.72 KB
PASSED: [[SimpleTest]]: [MySQL] 55,831 pass(es).
[ View ]

Let's get this done! :)

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Wims feedback got adressed

Wim Leers’s picture

Thanks :) And sorry for all the nitpicking! :(

RTBC +1

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

Needs reroll

curl https://drupal.org/files/1987140-34.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 10973  100 10973    0     0   114k      0 --:--:-- --:--:-- --:--:--  127k
error: patch failed: core/modules/edit/lib/Drupal/edit/Tests/MetadataGeneratorTest.php:9
error: core/modules/edit/lib/Drupal/edit/Tests/MetadataGeneratorTest.php: patch does not apply
error: patch failed: core/modules/editor/lib/Drupal/editor/Tests/EditIntegrationTest.php:9
error: core/modules/editor/lib/Drupal/editor/Tests/EditIntegrationTest.php: patch does not apply
damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new10.79 KB
PASSED: [[SimpleTest]]: [MySQL] 57,829 pass(es).
[ View ]

Rerolled.

Wim Leers’s picture

Status:Needs review» Reviewed & tested by the community

RTBC if comes back green.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 93e8015 and pushed to 8.x. Thanks!

Wim Leers’s picture

Issue tags:-sprint

.

Automatically closed -- issue fixed for 2 weeks with no activity.