Use direct methods instead of arrayAccess for display handlers

Rerolled.

To make sure we got them all, you can remove the ArrayAccess parts from PluginBag and see what blows up :)

Yeah, why not. This patch should actually work. Also attached one with the ArrayAccess methods removed from PluginBag.

We could do this? Although I don't think it's a great idea. This should work, but we have to initDisplay(). This should show that ht erest of your patch works atleast.

I would sooner wait for #1867304: Assess newDisplay method on ViewExecutable to get in, then I think the current patch will be ok.

Oops

Ok, rerolled. I ran some of the previously failing tests and they pass. So I think this patch might be good now.

That last patch was all messed up, it was chnaging back changes made from recent commits! Not sure how or why it got so messed up Let's try this.

I also changed the get method on the PluginBag to return by reference. Review carefully :)

And maybe now this; destroy() is unsetting displayHandlers, so we can't use has() to check displays. So I've Just changed the assertions to match the others...

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsDataTest.phpundefined
@@ -38,15 +31,10 @@ public function testViewsFetchData() {
-    $data = $this->viewsDataCache->get($this->randomName());
-    $this->assertTrue(empty($data), 'Make sure fetching views data for an invalid table returns empty.');

We're losing one assertion here, any particular reason? In fact, this whole hunk doesn't look related to the issue.

+++ b/core/modules/user/user.installundefined
@@ -690,7 +690,7 @@ function user_update_8009(&$sandbox) {
-function user_update_8011() {
+function user_update_8010() {
   global $user;
 
   // User pictures can only be migrated to the new user picture image field
@@ -846,7 +846,7 @@ function user_update_8011() {

@@ -846,7 +846,7 @@ function user_update_8011() {
 /**
  * Migrate {users}.picture to 'user_picture' image field.
  */
-function user_update_8012(&$sandbox) {
+function user_update_8011(&$sandbox) {
   // Initialize total values to process.
   if (!isset($sandbox['total'])) {
     $sandbox['total'] = (int) db_query('SELECT COUNT(picture) FROM {users} WHERE picture > 0')->fetchField();
@@ -914,7 +914,7 @@ function user_update_8012(&$sandbox) {

@@ -914,7 +914,7 @@ function user_update_8012(&$sandbox) {
 /**
  * Deletes {users}.picture field.
  */
-function user_update_8013() {
+function user_update_8012() {

These look like stray changes.

---

Can we also lump in the ArrayAccess removal from PluginBag again? We might as well kill it before others try to use it, and we can be sure we've 100% converted everything.

Good points, If no one else does first, I will look at this in about an hour. I knew there was some crazy stuff going on with the changes, indeed unrelated, that's why I said review carefully :) Good job.

Made those changes. Sorry I've just realised I forgot an interdiff, how rude.

As a punishment for forgetting an interdiff, you have to reroll!

You missed removing implements \ArrayAccess from the class definition, as well as the comment above it.

Ahahaha, fair IS fair. Here you go.

Ah, we just missed one direct call to offsetGet() that should never have existed.

I've reviewed the entire patch several times, and this is done.

In PluginBagTest::testPluginBag()

      $this->assertTrue(isset($plugin_bag[$instance_id]), format_string('Plugin instance @instance_id exits on the bag', array('@instance_id' => $instance_id)));
      $this->assertTrue($plugin_bag->has($instance_id), format_string('Plugin instance @instance_id exits on the bag', array('@instance_id' => $instance_id)));

Delete the first one, and s/exits/exists/ on the second

Not sure about the other failure

Sorry, I think is still from the messed up diff I provided earlier in the issue. This should be good now.

#32: drupal-1876942-32.patch queued for re-testing.

Just a patch context conflict with #1821524: Decouple use of default views in ViewStorageTest, no changes.

Committed/pushed to 8.x, thanks!

Super.