Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(views): refactor views system to store absolute paths #7070

Closed
wants to merge 2 commits into from

Conversation

ewinslow
Copy link
Contributor

TODOs:

  • Add tests for global code that depends on this service.
  • Add tests for integration with simplecache
  • Remove special-case for core views in getViewLocation
  • Update dev tools view inspector to handle the new way views paths are stored.
  • Throw \IllegalArgumentException for invalid viewtypes so we don't need so many null checks

Testdox output:

Elgg\Views\Registry

  • Registers php files as views
  • Registers static files as views
  • Get or create viewtype always returns the same instance for the same string
  • Current viewtype is the explicitly set viewtype if provided
  • Current viewtype falls back to the view input parameter if an explicit viewtype was not set
  • Current viewtype falls back to the db configured default if an input parameter was not provided
  • Current viewtype falls back to default if no other value was provided
  • Views are rendered with the current viewtype by default
  • Views are rendered with the specified viewtype as long as it is valid
  • Views are not rendered recursively with a specified viewtype if that viewtype is not the current one
  • Specified vars are available to views in the vars array
  • Global config being available via the vars array is deprecated
  • Current user being available via the vars array is deprecated
  • Site url being available via the vars array is deprecated

Elgg\Views\View

  • Uses php to render non static views
  • Does not use php to render static views
  • Can register views as cacheable
  • Static views are always cacheable
  • Can extend views
  • Extending a view with priority less than 500 prepends extension content
  • Extending a view without a specified priority appends extension content
  • Extending a view with priority greater than or equal to 500 appends extension content
  • Unextending a view removes the effect of a previous extension
  • Unextending a view has no effect on subsequent extensions
  • Views can be extended twice by the same view
  • Views extended twice by the same view need to be unextended twice to remove those extensions
  • Views can exist based on viewtype fallback
  • Views can exist based on extension

Elgg\Views\Viewtype

  • Viewtypes can fall back

This is a major refactoring of Elgg_ViewsService to remove global state and prep for #6844 (and #3737 to a lesser extent). I know for sure that the global functions are broken as a result of this, but we don't have tests for them. Looking for early feedback, especially in regard to what tests we can add.

DO NOT MERGE

@ewinslow ewinslow changed the title chore(views): refactor Elgg_ViewsService to store absolute paths chore(views): refactor Elgg_ViewsService to store absolute paths Jul 17, 2014
@ewinslow
Copy link
Contributor Author

I'm submitting this against 1.x even though this is technically an internal refactor because I am not confident about our test coverage. Main priority here IMO is getting more tests so all behaviors are covered and we can make this change with confidence.

}
return $this->file_exists_cache[$path];
private function getTemplateHandler() {
return isset($this->config->template_handler) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO bad place to use a ternary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can change to:

if (!isset($this->config->template_handler) ||
    !is_callable($this->config->template_handler)) {
  return '';
}

return $this->config->template_handler;

@mrclay
Copy link
Member

mrclay commented Jul 17, 2014

Besides making dependencies explicit and moving view storage out of $CONFIG, does this do anything else big?

@ewinslow
Copy link
Contributor Author

It stores absolute paths to each view instead of just storing the directory where the view can be found.

We'll need to update developer tools and possibly simplecache to handle that.

@ewinslow ewinslow assigned ewinslow and unassigned ewinslow Jul 27, 2014
@ewinslow
Copy link
Contributor Author

ewinslow commented Aug 7, 2014

The latest iteration of this has the following changes:

  • Created a new Elgg\Views namespace and moved the service there.
  • Created a View model class that contains some single-view-specific logic
  • Created a Viewtype class that contains logic for validating views + storing fallbacks

The View and Viewtype classes are the cleanest now, and the Service contains:

  • all the string->instance mappings
  • deprecation cruft
  • mapping from view names to view instances
  • plugin hook triggers

View instances don't know about their names at all. Viewtypes still do, though.

@ewinslow
Copy link
Contributor Author

ewinslow commented Sep 8, 2014

Pushed another update that renames Service to Registry. I find the latter more descriptive. Also fixes most of the existing tests.

@ewinslow ewinslow changed the title chore(views): refactor Elgg_ViewsService to store absolute paths chore(views): refactor views system to store absolute paths Oct 31, 2014
@ewinslow ewinslow removed this from the Elgg 1.10.0 milestone Dec 3, 2014
@ewinslow ewinslow force-pushed the views-b6844 branch 12 times, most recently from b53939e to 90a0a71 Compare January 26, 2015 03:04
@ewinslow
Copy link
Contributor Author

Replaced by #8390

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants