Skip to content

Conversation

matt-bernhardt
Copy link
Member

@matt-bernhardt matt-bernhardt commented Mar 27, 2023

The commits in this PR do the following:

  1. Copy-pastes the PHP templates from the legacy theme into this application, as well as one function definition that gets called in inc/entry.php. No code is changed. The templates which Pages will use do move into a new templates/ folder, similar to how the other themes are structured.
  2. Runs the automated linting tool PHPCBF to fix things like whitespace and indenting problems. This tool is pretty conservative, only making changes which will have no impact on code execution.
  3. Bumps the theme version to 0.1.0
  4. Explicitly loads an internally-developed PHP "library" named cards.php
  5. Fixes a duplicate template name in page-news.php
  6. Comments out a few calls to the removed getRoot function at the top of some page templates. This will need to be more thoroughly cleaned in the next PR.
  7. Adds the Mitlib/News namespace to any template that gets loaded when the production content gets imported.

The goal of all of this is to bring in all the legacy PHP templates, and to make sure that the resulting theme doesn't crash egregiously after the current set of content is imported.

This PR will not close LM-143 entirely, but it does bring us close enough that I'll be able to import the production content in a review app, and closely inspect the behavior and resolve the rest of the issues.

Developer

Secrets

  • No secrets are affected

Documentation

  • No documentation changes are needed

Accessibility

  • ANDI or Wave has been run in accordance to
    our guide and
    all issues introduced by these changes have been resolved or opened as new
    issues (link to those issues in the Pull Request details above)

Stakeholder approval

  • Stakeholder approval is not needed

Dependencies

NO dependencies are updated

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • The changes have been verified
  • The documentation has been updated or is unnecessary
  • New dependencies are appropriate or there were no changes

This includes:
- Page templates going into a templates/ directory
- Base templates staying at the theme root
- inc/ and lib/ folders holding partials and internal libraries
- The definition of the excerpt() function in functions.php

There are no code changes through this commit - this is copy-paste only,
although as noted above some files are now in a new location.
The changes here should be solid - I've come to trust PHPCBF's processes.
Most of the changes are likely for whitespace issues, like fixing code indenting and spacing requirements.
There already is a "Bibliotech" template, and this one is properly named "News" (for now)
This will be removed more completely later, but for now I'm just commenting it out.
This also adds a root namespace slash to calls which need to stay in the root namespace.

Some page templates are not affected by this change, because I haven't seen them get called yet.
@matt-bernhardt matt-bernhardt requested a review from JPrevost March 27, 2023 22:01
@matt-bernhardt
Copy link
Member Author

@JPrevost I've tagged you for code review, to start the conversation about this change with an ability to see what it is. I see that GitHub Actions and CodeClimate are freaking about about this, and there is technical debt still to be resolved.

If you'd prefer that I untag you and work on these flags, I'm happy to do so. I'm still working on the news theme, but am trying to prevent a ridiculously unwieldy PR. This may be too small a change, though, with this many issues remaining.

@JPrevost JPrevost self-assigned this Mar 28, 2023
@matt-bernhardt matt-bernhardt merged commit 6a8e719 into master Mar 28, 2023
@matt-bernhardt matt-bernhardt deleted the lm143 branch March 28, 2023 19:31
This was referenced Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants