We should have more setters/getters for the important stuff.

Postponing until we stop moving everything around :)

CommentFileSizeAuthor
#3 views-1792844-3.patch89.93 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » views.module
Priority: Major » Normal
Issue tags: +VDC
damiankloip’s picture

We talked about this on IRC, but for the benefit of this issue;

This could be pretty tricky at the moment, as the config entity controller uses Reflection to save all the public properties from ViewStorage into config. This was one of the reasons we made $executable protected when we split the classes and add a getExecutable to access it.

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
89.93 KB

This blocks #1798574: Refactor Views UI to be a form controller.

Because CMI can only write public properties to YAML files, we can't actually make these protected.

In the meantime, we can use setters and getters everywhere.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great cleanup! I kind of dislike that we have to first store a variable before using !empty but that's life.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I was confused about this patch (a little bit more context in these patches for people no on the VDC team would be helpful), but tim explained in irc that Views is trying to move away from being able to use $views->foo directly, both for proper refactoring of Views UI, as well as for translatable stuff, etc.

I asked why it was onyl covering a couple of things ('name', base_name') rather than everything and was told that it's because it's focused only on the public properties of a view for now. Further clean-up will happen in subsequent patches.

Since this is just basically a find/replace job, looks good here. I confess I didn't check each hunk individually but I'm guessing any issues will expose themselves pretty fast. ;)

Committed and pushed to 8.x (minus a hunk about the code that was committed as part of #1810826: Remove obsolete form code from views_ui/admin.inc). Thanks!

Status: Fixed » Closed (fixed)

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

xjm’s picture

Status: Closed (fixed) » Active
Issue tags: +Needs change record
xjm’s picture

Title: Change more public properties to protected in ViewExecutable and ViewStorage » [Change notice] Change more public properties to protected in ViewExecutable and ViewStorage
Project: Drupal core » Views (for Drupal 7)
Version: 8.x-dev » 8.x-3.x-dev
Component: views.module » Documentation
Assigned: tim.plunkett » Unassigned
Chris Matthews’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.7.x-dev
Component: Documentation » views.module
Issue summary: View changes

For more information as to why this issue was moved to the Drupal core project, please see issue #3030347: Plan to clean process issue queue

Chris Matthews’s picture

Project: Drupal core » Views (for Drupal 7)
Version: 8.7.x-dev » 7.x-3.x-dev
Component: views.module » Code
Status: Active » Closed (outdated)

Moving back to the contributed Views issue queue and closing as outdated per https://www.drupal.org/project/views/issues/3030347#comment-13023447